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

Regularly check updates for pineappl and eko dependencies #190

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Radonirinaunimi
Copy link
Member

I think this is an easy way to address #189. With this, updates will open a PR and in turn trigger the regression tests and benchmarks.

@scarlehoff
Copy link
Member

scarlehoff commented Aug 17, 2024

thanks! This should be coupled with a DIS test (with and without nfonll) and a hadronic (the lhcb dataset there might be enough). nfonll and hadronic run with the nnpdf interface (ideally th 410000000 as it will be our default going forward)

@felixhekhorn
Copy link
Contributor

thanks! This should be coupled with a DIS test (with and without nfonll) and a hadronic (the lhcb dataset there might be enough). nfonll and hadronic run with the nnpdf interface (ideally th 410000000 as it will be our default going forward)

maybe we should do the benchmark in another PR? as this one specifically addresses the dependency issue

I'm not 100% sure this is what we want ... this links pineko to the latest features in eko (and pineappl) and I'm not sure this is wise. After the Rust dust has settled (which still might take some time), maybe we also reached a stable phase in eko but it might still be better to perform a manual upgrade of the dependencies ...

What @scarlehoff is correctly concerned about (if I understood correctly) is that people spend a (large) amount of time on computing FK tables but they are useless in a fit. So we maybe rather want to check this statement? This will partly be addressed in the benchmark. A second step would be to then use these FK tables inside a quick nnpdf run.

(On the other side: we have pinned most(?) versions everywhere and we try to follow Semver, so it should be clear when we break, no?)

@scarlehoff
Copy link
Member

maybe we should do the benchmark in another PR? as this one specifically addresses the dependency issue

No, they are the same issue. We want to check when the dependencies break things.

I'm not 100% sure this is what we want ... this links pineko to the latest features in eko (and pineappl) and I'm not sure this is wise.

Well, it is either that or things will diverge quite a bit

What @scarlehoff is correctly concerned about (if I understood correctly) is that people spend a (large) amount of time on computing FK tables but they are useless in a fit. So we maybe rather want to check this statement? This will partly be addressed in the benchmark. A second step would be to then use these FK tables inside a quick nnpdf run.

No. In the NNPDF code I'm being quite proactive in making sure things are either up to date and when possible backwards compatible ( if e.g., pineappl 0.8 had broken the previous fktables I'd have been quite upset).
What I'm concerned about is FK Tables being wrong (like the DIS were in the other branch) and that can only be checked by a regression test here. If things change it needs to be known as soon as possible (like we do with the fitting code...*)

On the other side: we have pinned most(?) versions everywhere and we try to follow Semver, so it should be clear when we break, no?

If eko were backwards compatible I'd accept this, but the problem is we cannot pin eko because soon the ekos in the server will no longer be compatible with the version of eko that was pinned here (or in the nnpdf code). So it is important that everything gets updated as soon as possible and together.

*actually, the two bugs in the fitting code we had the last year were people saying "it's just numerics" and regenerating the regression tests :___

@scarlehoff scarlehoff mentioned this pull request Sep 12, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants