-
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
Allow evolution and convolution with different ekos for a single grid #181
Conversation
Do you want to make this a draft or shall I comment right away? |
Please feel free to comment, but note that this are dirty changes and ugly solutions, so be patient 😬 |
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.
As soon as this is ready and good to go (and perhaps modulo some cosmetic changes), we can test it with NNPDF/pineappl#289 - which should already give exactly what we want.
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
For the benchmarks, we don't necessarily need a new pineappl release. For testing purposes and until NNPDF/pineappl#289 is merged we can do the following in pineappl = { git = "https://github.com/NNPDF/pineappl.git", branch = "extend_eko_convolution", subdirectory = "pineappl_py" } |
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.
The benchmarks still fail when I tried locally because of some breaking changes that have been introduced recently (?). In particular regarding the renaming of lumi->channel
: NNPDF/pineappl@aef0982
cc @cschwan
Co-authored-by: Tanjona Rabemananjara <rrabeman@nikhef.nl>
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
The inversion method is also different. I'm running the ekos now with this branch so that I can give you the objets that reproduce the bug on my side. |
I can reproduce the error with a new environment: pip install .[nnpdf]
pineko fonll tcards 41000000
pineko fonll ekos 41000000 HERA_NC_318GEV_EM-SIGMARED --overwrite
pineko fonll fks 41000000 HERA_NC_318GEV_EM-SIGMARED --overwrite ekos, fktables and other artifacts can be found here: https://cernbox.cern.ch/s/RAADfwb3qk6HjyK (there's a few empty folders because there are inheritances and it doesn't work well when I copy the links from envPackage Version absl-py 2.1.0 |
@scarlehoff, so now with the template card matching exactly your settings, eko from this branch, I'll now inspect your ekos... difference old grid, new eko
|
Are we using the same version of Before we continue, if you do
from Results
Just to discard that it is a problem of the metadata of the final fktable. |
So the bug was indeed when using the old grids and generating opcards. |
Thanks @giacomomagni, I can confirm this fixes the issue for me! |
thanks @giacomomagni ! good job - and indeed your conjecture was correct. Can this then be merged? |
I'd like to go through it one more time |
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.
Thanks for this. Since this has by now been tested in a variety of scenarios I'm relatively sure it will be safe but I'm redoing theory 41000000
to see whether something changed in the final FKs, better safe than sorry.
My comments are mostly in the direction of future maintainability when someone else is tasked with pineko, given that afaia half of the people that have participated in this PR will not be with us in about six month time
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
Co-authored-by: Juan M. Cruz-Martinez <juacrumar@lairen.eu>
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.
Thank you very much @giacomomagni and apologies for the wait, I can reproduce 41_000_000 from master to whatever np.allclose
precision gives.
Before merging, I see that both @felixhekhorn and @andreab1997 had requested changes. From his last message I guess @felixhekhorn is ok with merging, not sure about @andreab1997 ? |
Give me just the time to have a look and I 'll let you know |
Adress #180.