-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update nnpdf40 runcard and add theory 40_000_000 #2082
Conversation
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
Comparison https://vp.nnpdf.science/2Fw_aIEDT3amjUe8rDsBBA== This is ready now. |
|
||
sampling: | ||
separate_multiplicative: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to get the exact comparison of my comment but I believe that this should be the default going forward
(it should not be a big or relevant effect and I guess you are using it anyway, but it could have an effect in the closure tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe that we should remove this key now, and just enforcing the correct behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this conversation I understood the default would be set to False such that it would allow us to remove these lines from the runcard, but instead just a comment was added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment in this PR and we take the default out in a different one (actually, @andreab1997 if you want to do the honors :P)
As promised in last week's PC.
Incidentally, this PR also solves a security issue we had in the code.
Fixes #2075
I'd like to run a fit to check that everything is ok but don't have the resources available atm. Will come later today or tomorrow.