-
Notifications
You must be signed in to change notification settings - Fork 10
Use eko v0.15 #2181
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
Use eko v0.15 #2181
Conversation
Tests that have been performed
Note, for eko == 0.13.4 eko cannot perform the rotation as the |
In principle we could now continue with this using the master branch from eko, right @felixhekhorn ? If @giacomomagni is fine with it, I'll rebase on top of master and resolve the conflicts so that this is ready to merge when the release of eko is out. |
yes that should work now!
Okay! |
Do we have an actual "official" theory with a 0.15 eko already? (otherwise I'll compute one with the latest commit of eko) |
Not yet, mine were unofficial for sure!! |
if you want an official one, we should tag eko - which I believe we can do and then we will break in v0.16 again (e.g. NNPDF/eko#441 is an "open" breaking change and would a bugfix, e.g. NNPDF/eko#447 , also be one?) |
I think both the changes you linked would be very easy to work around. By official I meant "used in a paper" anyway. |
afaics I get the exact same PDF with 0.14 and 0.15 ekos I should test it also with a QED eko I guess, but I'll wait for the release to do that test... |
Looking at the error in the CI, it seems to be due to the "low precision ekos", which were ekos for which the internal interpolation grid has only 15 points. This turns out to be a spurious error (the xgrid reshape should not trigger) because of a combination of https://github.com/NNPDF/eko/blob/c69a7b964a68342cfac728627be1a2c1084287ed/src/eko/io/v1.py#L26 and the eko for theory_399 being 0.13.4, for which Besides that, (cc @felixhekhorn) I think it would be good for EKO to provide a |
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
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.
If this works, I think it is fine for the moment - there is room for future improvements left
n3fit/src/evolven3fit/q2grids.py
Outdated
``Q2GRID_DEFAULT``: default NNPDF Q2 grid for evolution (55 points, starts at Q=1GeV) | ||
``Q2GRID_NNPDF40``: q2 grid used in the fits for the NNPDF4.0 release (49 points, starts at Q=1.65 GeV) | ||
``Q2GRID_Nf03``: q2 grid used in the perturvative charm fits for the NNPDF4.0 release (48 points, starts at Q=1GeV) | ||
``Q2GRID_DEFAULT``: default NNPDF Q2 grid for evolution (55 points, starts at Q=1GeV) |
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.
``Q2GRID_DEFAULT``: default NNPDF Q2 grid for evolution (55 points, starts at Q=1GeV) | |
- ``Q2GRID_DEFAULT``: default NNPDF Q2 grid for evolution (55 points, starts at Q=1GeV) |
you want a list, no?
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.
boh, do I?
I think the evolven3fit module is not being parsed by the docs anyway :__
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.
that's not a reason to not write proper docs 🙃 - I read them and I have a built-in rst parser, which runs faster if list are easy recognisable as such 😇
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 see (I didn't write them though, they just got caught by one of the pre-commit hooks).
I'll update the docs so that evolven3fit is also parsed so I can catch these as well.
for target_key in eko_op.operators: | ||
elem = eko_op[target_key.ep] | ||
|
||
if eko_op.metadata.version == "0.13.4": |
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.
Do we need to support 0.13.4? EKO itself currently only supports >=0.13.5
cc @evagroenendijk
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 was debating myself whether to include this or not, but in the end I decided to be nice. There are some theories people use that have 0.13.4, and this will let them use it for some time (unless you are now forbidding these for being read at all, when I tested this PR they were read but they were not 100% correct).
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.
Looking to NNPDF/eko#448 I think they are still read - @evagroenendijk do you agree?
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 think indeed they are read but a while ago when I tested versions <0.13.5 did not exactly reproduce the benchmark
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
Greetings from your nice fit 🤖 !
Check the report carefully, and please buy me a ☕ , or better, a GPU 😉! |
@felixhekhorn should I understand your last msg as an approval? |
This allows the usage of this PR from eko NNPDF/eko#415
Which makes (in my computer) a 100 replica fit evolve in ~2 minutes (instead of ~20m) (which would make #2168 unnecessary) but requires also NNPDF/eko#417
So I'm leaving this PR here and depending on how things work out in eko we will merge this directly or #2168 first and then revert it when we merge this.