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

Inconsistent convolution information: convolution_particle vs. initial_state #316

Closed
LucianHL opened this issue Oct 4, 2024 · 16 comments
Closed
Assignees

Comments

@LucianHL
Copy link

LucianHL commented Oct 4, 2024

Hi, this is quite possibly only ever going to be an issue for me, as I have been converting some potentially reasonably old applgrids to pineappl grids. But in the first example I have tried with the NNPDF code for some reason in the configs data of the converted file convolution_particle is used for beam 1 and initial_state for beam 2, i.e. there is convolution_particle_1 and initial_state_2. This is causing pineparser.py to crash when these are read in. Possibly this is better fixed at the conversion stage but in any case changing the relevant try statements to:

try:
    parton1 = pine_rep.key_values()["convolution_particle_1"]
except KeyError:
    parton1 = pine_rep.key_values()["initial_state_1"]

try:
    parton2 = pine_rep.key_values()["convolution_particle_2"]
except KeyError:
    parton2 = pine_rep.key_values()["initial_state_2"]

fixes it, so might be worth updating.

@scarlehoff
Copy link
Member

scarlehoff commented Oct 4, 2024

@Radonirinaunimi can we have cases where the two are mixed? (probably a bug in pineappl? I know it happened at some point for tevatron grids due to a bug in the charge conjugation, but maybe there's another one lurking?)

@LucianHL
Copy link
Author

LucianHL commented Oct 4, 2024

The grid is ATLAS 7 TeV TTbar total cross section if that helps

@Radonirinaunimi
Copy link
Member

Hi @LucianHL, thanks a lot for making us aware of this! Would it possible for you to share with us the APPLgrid you started from so that I can have a closer look?

@Radonirinaunimi can we have cases where the two are mixed? (probably a bug in pineappl?

In principle these two should never be mixed as it might lead to other more subtle issues. I believe that given that the APPLgrid and FastNLO converters are not actively used there might be some issues we did not encounter before there.

@LucianHL
Copy link
Author

LucianHL commented Oct 4, 2024

Thanks - this seems to be happening irrespective of the input applgrid actually (well, I have tried two so far). But here is the TTbar one, along with the converted pineappl grid.

This has been done using the pineappl cargo installation:

cargo install --locked --features=applgrid pineappl_cli

and then the pineappl import command. Let me know if you need any more information

@Radonirinaunimi
Copy link
Member

Perfect, thanks! I will have a look and will get back to you soon.

@scarlehoff
Copy link
Member

Let me move this to the pineappl repo for organizational purposes

@Radonirinaunimi whether to add the code in the OP to pineparser.py here I leave it to you (i.e., whether you prefer to make sure that "broken" grids do not work so that they need to be fixed in some way; I favour a bit this option but woudn't mind putting the if-else that @LucianHL wrote if that's easier)

@scarlehoff scarlehoff transferred this issue from NNPDF/nnpdf Oct 4, 2024
@Radonirinaunimi
Copy link
Member

@Radonirinaunimi whether to add the code in the OP to pineparser.py here I leave it to you (i.e., whether you prefer to make sure that "broken" grids do not work so that they need to be fixed in some way; I favour a bit this option but woudn't mind putting the if-else that @LucianHL wrote if that's easier)

Just to say that I also favor the option of not allowing these mix in pineparser.py so we can keep the current way as it is.

@LucianHL
Copy link
Author

LucianHL commented Oct 4, 2024

Thanks, just to confirm that checking some other grids these all have the issue so it is not specific to the input applgrid but rather the conversion itself by the look of it.

@t7phy
Copy link
Member

t7phy commented Oct 4, 2024

Hi @LucianHL , this ofc needs to be fixed from our end, but I have a quick solution for you in case it helps. This information is located in the metadata which can be read using pineappl read --show <grid-name>. This is what I get:

$ pineappl read --show grid-total-TTbar_ATLAS-7TeV.pineappl.lz4 
convolution_particle_1: 2212
convolution_type_1: UnpolPDF
initial_state_2: 2212
pineappl_gitversion: cargo:0.8.3

Now, using pineappl write, you can edit this metadata. If you run this:
$ pineappl write --delete-key initial_state_2 --set-key-value convolution_particle_2 2212 --set-key-value convolution_type_2 UnpolPDF grid-total-TTbar_ATLAS-7TeV.pineappl.lz4 fixed-grid.pineappl.lz4, you would get a fixed-grid.pineappl.lz4 that should have the following keys:

$ pineappl read --show fixed-grid.pineappl.lz4 
convolution_particle_1: 2212
convolution_particle_2: 2212
convolution_type_1: UnpolPDF
convolution_type_2: UnpolPDF
pineappl_gitversion: cargo:0.8.3

This should now work 🤞

@LucianHL
Copy link
Author

LucianHL commented Oct 4, 2024

Thank you @t7phy - yes that works as you suggested and no problem to work that into the conversion script for now. I will let you know if I have any issues running with these

@cschwan
Copy link
Contributor

cschwan commented Oct 4, 2024

This problem has been reported before, but unfortunately I didn't find an Issue, which probably means it doesn't exist. So thank you for your report, @LucianHL!

The bug has been fixed in commit 741dcb9, but there isn't a release with the fix yet. You probably don't want to use master, because it (intentionally) breaks the Python interface. I'll release v0.8.4 with this fix soon.

@cschwan cschwan changed the title convolution_particle vs initial_state issue Inconsistent convolution information: convolution_particle vs. initial_state Oct 4, 2024
@cschwan
Copy link
Contributor

cschwan commented Oct 4, 2024

Here's a new release: https://github.com/NNPDF/pineappl/releases/tag/v0.8.4. @LucianHL, if you have the time please test whether this release has indeed fixed the observed behaviour. If that's the case, feel free to close the Issue.

@cschwan cschwan self-assigned this Oct 4, 2024
@LucianHL
Copy link
Author

LucianHL commented Oct 4, 2024

Thank you I will do - will that work with the cargo install above or do I need to do something different?

@cschwan
Copy link
Contributor

cschwan commented Oct 4, 2024

It should work with all advertised installation methods, including cargo install!

EDIT: I forgot that you're mainly interested in the conversion of .root to .pineappl, which requires ROOT and that unfortunately only works with the source-based installation.

@LucianHL
Copy link
Author

LucianHL commented Oct 7, 2024

Thank you - yes that installs fine and produces grids with the right convolution_particle labelling

@LucianHL LucianHL closed this as completed Oct 7, 2024
@cschwan
Copy link
Contributor

cschwan commented Oct 7, 2024

Great!

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

No branches or pull requests

5 participants