Remove need for _template.yaml#223
Conversation
|
@scarlehoff Could I maybe just remove this test? I could make it check that the theory cards' Q0 is the same as the default one, but to me it seems a bit superfluous |
|
Sure, shuffling around the _template and the operators might change which tests are needed. |
felixhekhorn
left a comment
There was a problem hiding this comment.
some quick things I noticed last week
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
|
@felixhekhorn The regression benchmark comparing convolved predictions failed, but the maximal relative difference is 0.03497747. Would you say it's close enough? |
|
Let me know when I should try to test this again! Btw, perhaps it would be good to rebase on top of master (or merge the changes from master)? |
|
@scarlehoff Yes thanks, I implemented the changes you suggested and made the necessary adjustment, let me know if you agree! Sorry, I took the day off on Friday so I just finished it today |
|
Thanks, this now works for me in a folder with no template and It would be great if @kamillaurent could check explicitly that the fktables he computed didn't change. The best course of action is to select to datasets (one hadronic, one DIS, they can be quite small so that it goes faster), ideally from a theory with scale variations, and check that the results of the fktables ran with this branch are identical to the previous ones. |
|
@kamillaurent did you have a change to test this? |
|
@scarlehoff no, because I saw there was movement on the PR. Also I am working on PR #247 and I thought it would be easier to test this after merging the previous one (so I apply the changes on top of it and I test if everything is consistent). If for some reason PR #247 can't be merged in the short term, I will test this one without waiting for the other to be merged. |
|
@evagroenendijk is this one final? @kamillaurent I'm actually unsure what the best course of action is, because these two have conflicting changes. If you confirm that #247 is now final, I will merge that one and, if @evagroenendijk confirms this one is also final, we can merge master into this branch (I favour (but it is important that you test with an exact commit that is available the repo) |
|
I confirm that #247 is final, if you have no other change request. |
felixhekhorn
left a comment
There was a problem hiding this comment.
I dislike the fact that ipd=4 is now defined in several places, but for the sake of getting this PR done I will ignore it here for the moment ...
| @click.option( | ||
| "--ipd", default=4, show_default=True, help="interpolation polynomial degree" | ||
| ) | ||
| @click.option("--iil", default=True, show_default=True, help="interpolation is log") |
There was a problem hiding this comment.
why did you remove the options from the ekos command?
There was a problem hiding this comment.
I removed them from the fonll command because Juan Cruz mentioned that these options are not there for fonll (that there is only the option for n_cores)
There was a problem hiding this comment.
right, I forgot the above comment ... well then I'm not sure I agree, but for the sake of merging this ASAP we can leave as is and see if this generates problems (which it won't, I think, we never change these settings)
There was a problem hiding this comment.
I think there was a bit of miscommunication there ^^U
I meant there was no theory card option in the fonll command.
@felixhekhorn if we never change these settings then I would remove them also from the operator card command in the not-fonll runs.
There was a problem hiding this comment.
@felixhekhorn if we never change these settings then I would remove them also from the operator card command in the not-fonll runs.
I have never seen them being changed in real life ... for the sake of simplifying we can remove them also in the file-based CLI (either in this PR or in a separate one)
…1 in the operator card Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
@scarlehoff up to me this is final and can be reviewed! |
felixhekhorn
left a comment
There was a problem hiding this comment.
The _template.ci.yaml needs to be also removed from the server after merging
|
@kamillaurent please perform the discussed test and then we can merge! |
Remove the need for _template.yaml as in issue #202