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

Evaluate only unique xgrids #1587

Merged
merged 3 commits into from
Sep 1, 2022
Merged

Evaluate only unique xgrids #1587

merged 3 commits into from
Sep 1, 2022

Conversation

scarlehoff
Copy link
Member

Deals with #1586 in a very generic way so that one is still allow to mix situations in which not all xgrids are equal but some of them are.

For now only done for the fit. Once tests have run and it works there, I'll try to add some kind of cache to the vp PDF so that we can avoid calling lhapdf once per dataset (which I cannot promise because it is both trivial to do and trivial to do it in a disastrous way).

@scarlehoff scarlehoff linked an issue Aug 19, 2022 that may be closed by this pull request
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Aug 19, 2022
@scarlehoff
Copy link
Member Author

scarlehoff commented Aug 19, 2022

Actually, loading 100 times (order of number of datasets) the same 100 points xgrid/qgrid from 100 LHAPDF members doesn't seem to justify the added complication of caching the result, so probably it doesn't make sense to do it.

1.45 s ± 9.35 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I might play with it at some point in case it does improve things in real use cases.

@github-actions
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 added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Aug 19, 2022
@github-actions
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

Positivity seems ok (which is the only place where this would have an effect in the standard fit) https://vp.nnpdf.science/tmsUkZzYTfugL2uy5OoNfA==

@scarlehoff scarlehoff marked this pull request as ready for review August 22, 2022 08:54
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label Aug 22, 2022
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Aug 29, 2022
@scarlehoff
Copy link
Member Author

I'm not unhappy with the current version because it already works with some datases (i.e., positivity) of theory 200. So doesn't care about pineappl or not.

If you guys are ok with it, it can be merged.

If you still have doubts, we can wait as right now I have no idea about the gains that it would introduce as I need to be provided with smaller pineappl tables so that I can do a meaningful comparison. (in summary, I'm not dead set yet on this being useful beyond "oh cool, the pdf model is evaluated slightly faster")

@github-actions
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 Aug 31, 2022
@scarlehoff scarlehoff merged commit fe594f7 into master Sep 1, 2022
@scarlehoff scarlehoff deleted the evaluate_only_unique_xgrid branch September 1, 2022 16:21
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.

Reuse the same grid for all fktables when using a pineappl theory.
3 participants