Skip to content

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

Merged
merged 14 commits into from
May 14, 2025
Merged

Use eko v0.15 #2181

merged 14 commits into from
May 14, 2025

Conversation

scarlehoff
Copy link
Member

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.

@felixhekhorn felixhekhorn changed the title Use eko v15 Use eko v0.15 Oct 18, 2024
@scarlehoff
Copy link
Member Author

scarlehoff commented Nov 27, 2024

Tests that have been performed

  • Evolve fit with 0.13 ekos
  • Evolve fit with 0.14 ekos
  • Evolve fit with 0.15 ekos

Note, for eko == 0.13.4 eko cannot perform the rotation as the xgrid that eko v0.15 is able to read is the internal interpolation grid.

@scarlehoff
Copy link
Member Author

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.

@giacomomagni
Copy link
Contributor

In principle we could now continue with this using the master branch from eko, right @felixhekhorn ?

yes that should work now!

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.

Okay!

@scarlehoff
Copy link
Member Author

Do we have an actual "official" theory with a 0.15 eko already? (otherwise I'll compute one with the latest commit of eko)

@giacomomagni
Copy link
Contributor

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!!

@felixhekhorn
Copy link
Contributor

Do we have an actual "official" theory with a 0.15 eko already? (otherwise I'll compute one with the latest commit of eko)

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?)

@scarlehoff
Copy link
Member Author

I think both the changes you linked would be very easy to work around. By official I meant "used in a paper" anyway.

@scarlehoff scarlehoff marked this pull request as ready for review March 10, 2025 09:20
@scarlehoff
Copy link
Member Author

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...

@scarlehoff
Copy link
Member Author

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 xgrid was the internal interpolation instead of the externally-accessible grids. I'll create new "low precision theories" with newer ekos since 399/398 are outdated anyway, and for the time being I'll just work around that issue.

Besides that, (cc @felixhekhorn) I think it would be good for EKO to provide a xgrid_reshape which acts on the entire operator struct (and updates the metadata accordingly). The new interface is a bit clunky I feel and it is easy to forget steps (like updating the items in place or updating the xgrid in the operator, which were forgotten here indeed!)

@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Mar 24, 2025
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Mar 24, 2025
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Mar 25, 2025
Copy link
Contributor

@felixhekhorn felixhekhorn left a 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

``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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
``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?

Copy link
Member Author

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 :__

Copy link
Contributor

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 😇

Copy link
Member Author

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":
Copy link
Contributor

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

Copy link
Member Author

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).

Copy link
Contributor

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?

Copy link
Contributor

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

scarlehoff and others added 2 commits May 13, 2025 11:59
Co-authored-by: Felix Hekhorn <felixhekhorn@users.noreply.github.com>
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label May 14, 2025
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff
Copy link
Member Author

@felixhekhorn should I understand your last msg as an approval?

@scarlehoff scarlehoff merged commit 81c0da7 into master May 14, 2025
14 checks passed
@scarlehoff scarlehoff deleted the use_eko_v15 branch May 14, 2025 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-fit-bot Starts fit bot from a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants