search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Add support for APPLgrid conversion in `import` #161

Closed cschwan closed 1 year ago

cschwan commented 2 years ago

Added in commits 5941bf6dd4910c8dc996b2b6e92cbdad308d4172, af4047077da8686102f4f7defd7498fc9b5cb646, 42e4e16911b40e0c29986c6f715e525b12fcfc39, 052b077c530367d790b62664aa17e071fdaadae9 and cd854cbebe9826a41fbef20043f22b1c01c48253.

cschwan commented 2 years ago

@AleCandido @felixhekhorn could you give this a try (the git version, it's not released yet)? Note the installation instructions. This should work just as fine as the old converter, and even a bit faster because we no longer need to guess the reweight parameters thanks to commit 42e4e16911b40e0c29986c6f715e525b12fcfc39. Let me know if there's anything that I can improve.

alecandido commented 2 years ago

I guess I need root...

This week I'm at a conference, but I'll try to do it as soon as possible on dom, I guess there there might already be an installed instance of root.

cschwan commented 2 years ago

dom has root!

cschwan commented 2 years ago

The real advantage of these changes are that we're no longer restricted to use our NNPDF-modified version of APPLgrid; we can/must use the most recent version of it and also upgrade it in the future. I checked that the entire https://github.com/NNPDF/applgrids repository converts, and it seems that it even supports the photon grids out of the box.

felixhekhorn commented 2 years ago

If this would be a PR it would be much easier to review ;-)

felixhekhorn commented 2 years ago

Why do you need the APPL_IGRID_DIR variable? can't you expect it to be in CPATH (which I believe should be the default c/c++ search path )? or in case you can't maybe the example should use pkgconfig? Otherwise people might be confused "where did I install it again" and if you consider experimentalists running this then the installation might be somewhere far away (meaning out of control)

cschwan commented 2 years ago

Instead of APPL_IGRID_DIR you can indeed also use CPATH (if your compiler supports it). You can't use pkg-config because APPLgrid doesn't support it and in any case this header isn't installed, it's part of the private interface (that's why it is in the src directory).

The NNPDF-modified APPLgrid solved the same problem by making all the private headers public, but in this way every version you'd like to support you have to patch, and that's too fragile. APPL_IGRID_DIR isn't the best approach, but the least worst, in my opinion.

alecandido commented 1 year ago

Ok, I see it is unpleasant, but I guess I agree with @cschwan at this point. You don't want to forever patch APPLgrid... On the other side, if you are not the one that installed APPLgrid, most likely you're also not the guy that is going to make this non-minimal installation of PineAPPL as well.

cschwan commented 1 year ago

If there's any problem with the importer please open a separate Issue.