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

Add an lhapdf_compatibility module for LHAPDF #1799

Merged
merged 6 commits into from
Jan 31, 2024
Merged

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented Aug 25, 2023

This branch is now an update on top of #1861 that adds the possibility of running fits using pdfflow instead of lhapdf (during the fit lhapdf is used only for the t0 covmat and that should be covered fine by pdfflow).

Instead of directly importing lhapdf, the lhapdf imports are contained in an lhapdf_compatibility module which, if lhapdf is installed will simply provide it normally and transparently so nothing changes.

if lhapdf is not installed, it uses a combination of lhadpf_management and pdfflow to do whatever is needed from lhapdf.

wrt to those two libraries, lhapdf_management is feature complete in that it does all the management part of LHAPDF (installing, updating, looking around the metadata of a PDF) but it does nothing with the PDF themselves.
pdfflow instead is a port of the interpolation and extrapolation parts of LHAPDF but it is not really being maintained anymore. For what we need in the fit is enough and python-only (so we can run fits normally) but I would not swap LHAPDF for this library.

However, by concentrating all that is needed from LHAPDF in a single module it will be easier to swap LHAPDF for some other library that might appear in the future. We can also more easily extend LHAPDF. This will be relevant for instance for the nuclear PDFs.

The way I've included the changes has a nice perk (/cc @alecandido) which is the fact that one should be able to extract data/fktables from validphys without pdfflow or lhapdf (meaning, tensorflow is not needed for that).

@scarlehoff scarlehoff mentioned this pull request Aug 25, 2023
@scarlehoff scarlehoff linked an issue Aug 25, 2023 that may be closed by this pull request
@scarlehoff scarlehoff force-pushed the run_wo_lhapdf branch 2 times, most recently from d466052 to 73bb344 Compare November 17, 2023 16:26
@scarlehoff scarlehoff added the devtools Build, automation and workflow label Nov 27, 2023
@scarlehoff scarlehoff changed the base branch from master to change_installation_method November 27, 2023 15:04
@scarlehoff
Copy link
Member Author

This now could actually be merged (after #1861 of course since it relies in the pyproject.toml installation)

@RoyStegeman
Copy link
Member

Thanks, I'll have a look later but for now - do you want to keep lhapdf_management external to nnpdf?

@alecandido
Copy link
Member

It's great, thanks @scarlehoff!

I believe we might want to keep an optional dependency on LHAPDF as long as it will be used (so almost forever, from present perspective), to test the grids produced.
But this should not interfere with the fit, so the lhapdf_compatibility approach is great.

In particular: lhapdf_compatibility is greater for transitioning, but the moment we'd have another reliable replacement, whatever it is (if ever), we do not even need to keep it side-by-side with lhapdf.

@scarlehoff
Copy link
Member Author

Thanks, I'll have a look later but for now - do you want to keep lhapdf_management external to nnpdf?

Yes.

I believe we might want to keep an optional dependency on LHAPDF as long as it will be used (so almost forever, from present perspective), to test the grids produced.

Yes, for now the dependency on LHAPDF is mandatory from conda (and so when using the official distribution of nnpdf). This PR just opens the possibility of not using it (i.e., in development environments).

In order to use this PR you need to install the code like pip install .[nolha]

@scarlehoff scarlehoff force-pushed the change_installation_method branch 2 times, most recently from 0361733 to 3b7d9a1 Compare December 13, 2023 15:43
@scarlehoff scarlehoff force-pushed the change_installation_method branch 2 times, most recently from 77f3dec to 52d3a6f Compare December 21, 2023 11:14
@scarlehoff scarlehoff force-pushed the change_installation_method branch from 52d3a6f to a0f850e Compare January 17, 2024 08:25
Base automatically changed from change_installation_method to develop_20240119 January 19, 2024 12:05
Base automatically changed from develop_20240119 to develop_merge_20240119 January 19, 2024 13:39
@scarlehoff
Copy link
Member Author

@RoyStegeman did you have a chance to look at this?

@scarlehoff scarlehoff mentioned this pull request Jan 23, 2024
@scarlehoff scarlehoff force-pushed the develop_merge_20240119 branch from 595e3a9 to 8cb04b3 Compare January 24, 2024 08:50
Base automatically changed from develop_merge_20240119 to master January 24, 2024 09:32
@scarlehoff scarlehoff added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Jan 26, 2024
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Jan 30, 2024
@NNPDF NNPDF deleted a comment from github-actions bot Jan 30, 2024
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 😉!

validphys2/src/validphys/lhio.py Outdated Show resolved Hide resolved
validphys2/src/validphys/lhio.py Show resolved Hide resolved
validphys2/src/validphys/checks.py Outdated Show resolved Hide resolved
validphys2/src/validphys/lhapdf_compatibility.py Outdated Show resolved Hide resolved
Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

Everything looks okay. I'll have to take a bit of a leap of faith that pdfflow behaves the same as lhapdf and implements all the relevant functions, but the passing of the tests is a good sign.

And since lhapdf is still the recommended backend to interact with PDFs it's not our responsibility if this breaks things (right??)

n3fit/src/n3fit/backends/keras_backend/internal_state.py Outdated Show resolved Hide resolved
n3fit/src/n3fit/backends/keras_backend/internal_state.py Outdated Show resolved Hide resolved
validphys2/src/validphys/lhapdf_compatibility.py Outdated Show resolved Hide resolved
@scarlehoff
Copy link
Member Author

And since lhapdf is still the recommended backend to interact with PDFs it's not our responsibility if this breaks things (right??)

Yes. The idea is to be able to develop without having to have an lhpdf installation. This is particularly relevant when testing different version of python, you then need to install lhapdf on each of them even if you are just checking that e.g. validphys is creating the thcovmat in the right order.

@RoyStegeman
Copy link
Member

What about postfit (and some other minor scripts that still use classic lhapdf)?

@scarlehoff
Copy link
Member Author

scarlehoff commented Jan 31, 2024

What about postfit (and some other minor scripts that still use classic lhapdf)?

postfit (and reweighting and some other) will need lhapdf.
But these are scripts that generate a final lhapdf-product so I'd say it is fair.

but the passing of the tests is a good sign.

The tests are always using the official lhapdf installation.

@RoyStegeman
Copy link
Member

The tests are always using the official lhapdf installation.

Haha I realized my mistake

@scarlehoff scarlehoff merged commit 62556ce into master Jan 31, 2024
8 checks passed
@scarlehoff scarlehoff deleted the run_wo_lhapdf branch January 31, 2024 15:01
@alecandido
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
destroyingc++ devtools Build, automation and workflow run-fit-bot Starts fit bot from a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate pdfflow
3 participants