-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fixing integrability opcards #41
Conversation
Codecov Report
@@ Coverage Diff @@
## main #41 +/- ##
==========================================
- Coverage 44.53% 44.16% -0.38%
==========================================
Files 15 15
Lines 595 600 +5
==========================================
Hits 265 265
- Misses 330 335 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 would appreciate if your commit message contains more information then just "Fixing" 🙃 e.g. "Add int parameter to API" or "Propagate integrability info" or whatever ;-)
- if this fix is doing the trick - I'm happy to merge for now ...
for the future we may however reconsider:
- writing unit tests (if you have some spare time now, even do it now)
- and we should make ourselves aware, that this behaviour indeed relies on float arithmetic, which is potentially dangerous
- because the way we're hacking it, we're relying on the fact that EKO will return an identity and hence we can get away with what ever xgrid ...
- to be specific this relies on the fact that the number here https://github.com/NNPDF/runcards/blob/3a5b7c9dc053707bb74d181c69b5add50c5a385f/pinecards/NNPDF_INTEG_XT3_40/integrability.yaml#L4 is inside float equality equal to Q0^2 in the theory card (which is even outside our scope)
Yes sorry but they were just some minor stuff I forgot.
I am testing now |
Ok I can confirm it works, should I merge? @felixhekhorn |
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.
actually, inside the theory command we should determine the "integrability-property" from the grid - those grids have a marker: the integrability_version
I guess this works even for the file based API since at opcard generation time you need to have a grid present |
Right, this seems a better solution than asking for a flag |
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 like short PRs ;-)
@scarlehoff is this ok to merge for you? |
We realized that the opcards for the integrability grids were wrongly generated by pineko. In particular the point in targetgrid was not contained in the interpolation_x_grid list.