Skip to content
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

Support reading v0.13 and v0.14 files #417

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

felixhekhorn
Copy link
Contributor

@felixhekhorn felixhekhorn commented Oct 17, 2024

Okay, this will need some thinking, as we maybe want to come up with a more future-proof strategy (see e.g. #145 #328)

  • this drops the Legacy runner, i.e. closes Drop legacy runner #398 - this will break many things potentially again, i.e. this is surely another major break to master. eko.runner.managed.solve which is the new default does only accept new runcards, i.e. if you need to convert the user has to do that before calling the function (see also below)
  • this drops the old file converter which was for files before v0.11 (I think)
  • then continuing from Legacy to versions #175:
    • we still have eko.io.runcards.Legacy: 1. we should rename that to NnpdfDb or similar (and maybe move it?) and 2. I guess we need to maintain that also midterm (or even forever)
    • we haven't increased the __data_version__ since we introduced it, but I guess we should start doing so to disentangle a change in API (__version__) from a change in the file format (__data_version__). The latter also refers any file in the tar, i.e. including all .yaml files. We should increase it to 3 and rely on versions beyond v0.15 on that (1 ~ v0.13, 2 ~ v0.14 although the latter was never applied).
  • I don't know how to recover from Change manipulate to act on Operator #292 : old ekos might be rotated (on any of the four axes) and we would need to counteract this everywhere ... any ideas? It is true that the old format and the new format are technical the same, but the content might not

cc @scarlehoff


TODO:

  • unify loading of unit test assets; i.e. move https://github.com/NNPDF/eko/blob/master/crates/dekoder/tests/data/download.sh somewhere more global and use it for both Rust and Python unit tests
  • move all test assets to that script, i.e. to the server
  • test v0.13:
    • First create EKO object (the v0.13 test case) in v0.13, use LHA LO FFNS
      Do the following things for this EKO object in the newest EKO version from master branch:
    • reading of theory card: check it reads the theory card as a theory card
    • reading of operator card: check it reads the operator card as an operator card
    • reading of the 4D operator:
  • test v0.14 (same as for v0.13)
    • First create EKO object (the v0.13 test case) in v0.13, use LHA LO FFNS
      Do the following things for this EKO object in the newest EKO version from master branch:
    • reading of theory card: check it reads the theory card as a theory card
    • reading of operator card: check it reads the operator card as an operator card
    • reading of the 4D operator:
  • test v0.15 (same as for v0.13, but now reuse the existing v0.15 test case instead of making it again)
    Do the following things for this EKO object in the newest EKO version from master branch:
    • reading of theory card: check it reads the theory card as a theory card
    • reading of operator card: check it reads the operator card as an operator card
    • reading of the 4D operator:

@felixhekhorn felixhekhorn added refactor Refactor code output Output format and management labels Oct 17, 2024
@scarlehoff
Copy link
Member

(or even forever)

4ever

But I'm happy with moving it to the theory parser in NNPDF (i.e., having an attribute in the Theory file which is return_eko_theory or something).
But I would say let's do things incrementally, for now let's keep it here, let's make sure things are working and then we move it.

since we introduced it, but I guess we should start doing so to disentangle a change in API

I agree. The ones in 0.15 are surely a data version different from what we have

I don't know how to recover from

from the nnpdf side I don't mind not having this. In practice it would mean that the x-grid is completely fixed. If I understand correctly this feature has been more a convenience for people that could create more custom ekos and then got the 192x LHAPDF grid as the output? (did I understood it correctly @giacomomagni ?)

@giacomomagni
Copy link
Collaborator

  • this drops the Legacy runner, i.e. closes Drop legacy runner #398 - this will break many things potentially again, i.e. this is surely another major break to master. eko.runner.managed.solve which is the new default does only accept new runcards, i.e. if you need to convert the user has to do that before calling the function (see also below)

since we are updating evolven3fit we might consider to update as well pineko... but it will require a bit of work.

  • this drops the old file converter which was for files before v0.11 (I think)

here we are safe I think, it's already impossible to read 0.12 (?)

  • I don't know how to recover from Change manipulate to act on Operator #292 : old ekos might be rotated (on any of the four axes) and we would need to counteract this everywhere ... any ideas? It is true that the old format and the new format are technical the same, but the content might not

so in order to use the old ekos (0.13) you'd need to convert them, can't we add some checks while applying the conversion and warn the user?

@scarlehoff
Copy link
Member

so in order to use the old ekos (0.13) you'd need to convert them, can't we add some checks while applying the conversion and warn the user?

Wait, I thought the pdf ekos that we have were already ok without rotation? Or do you mean the fktables ekos?

@felixhekhorn
Copy link
Contributor Author

But I would say let's do things incrementally, for now let's keep it here, let's make sure things are working and then we move it.

definitely. I'm not sure, if that is better here or in NNPDF - we may want to move it e.g. into ekobox though

since we are updating evolven3fit we might consider to update as well pineko... but it will require a bit of work.

yes, but it will require anyway work since this is a major change already

here we are safe I think, it's already impossible to read 0.12 (?)

I think so - that was never requested and so with this PR I put a hard limit on v0.13

so in order to use the old ekos (0.13) you'd need to convert them, can't we add some checks while applying the conversion and warn the user?

just warning would be doable - but is that sufficient?

from the nnpdf side I don't mind not having this. In practice it would mean that the x-grid is completely fixed. If I understand correctly this feature has been more a convenience for people that could create more custom ekos and then got the 192x LHAPDF grid as the output? (did I understood it correctly @giacomomagni ?)

Wait, I thought the pdf ekos that we have were already ok without rotation? Or do you mean the fktables ekos?

that's the thing: eko can not know for what you use the it - and FK-ekos are rotated by default, even on multiple axes (I think). The main post-fit eko might be safe by chance. You can still rotate the eko in any way you like, but it is now always stored in the "canconical basis" (of course you can force your way)

@scarlehoff
Copy link
Member

Yes, they might be correct by chance... but if they are correct then we are happy I'd say, chance or not

@felixhekhorn
Copy link
Contributor Author

sorry @evagroenendijk for the delay but I added some todo items to the PR description above on which you can work as a warm-up; are the items sufficiently clear? Please ping me if they are not ...

@giacomomagni should we pin the test cases to LHA LO FFNS unpolarized? and check the actual numbers are fine? should we check for failure (i.e. "non-squared" or x rotated)?

@giacomomagni
Copy link
Collaborator

@giacomomagni should we pin the test cases to LHA LO FFNS unpolarized? and check the actual numbers are fine? should we check for failure (i.e. "non-squared" or x rotated)?

Yes that could be a good idea, I'd indeed try to check if the thing is not-squared, because it's not possible to translate it or use it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
output Output format and management refactor Refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop legacy runner
3 participants