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

`convolute_eko`: `x2_grid` was determined though not needed #167

Closed felixhekhorn closed 1 year ago

felixhekhorn commented 1 year ago

During the debug of https://github.com/NNPDF/runcards/pull/150 we realized there is a bug in PineAPPL convolute_eko: x2_grid was determined although it was never needed (because x2 is trivial)

This fix adds a protection to the generation same as there is already for reading: https://github.com/N3PDF/pineappl/blob/04650d99dd52f7985ba4fb2a976980b2b91ce6e7/pineappl/src/grid.rs#L1632

the quickfix (applied in https://github.com/NNPDF/runcards/pull/150/commits/3a5b7c9dc053707bb74d181c69b5add50c5a385f ) is to point the trivial point to an existing point in the x1 grid - this is done also for DIS (and everything derived from that), specificly x2=1, which, by chance, is in the x1 grid; that's also the reason why it was not discovered before

there might be an other bug related: @scarlehoff @andreab1997 I couldn't trigger again the core dump - can you remind me, what do I need to do?

codecov[bot] commented 1 year ago

Codecov Report

Merging #167 (dc33d2b) into master (8cc3bdd) will decrease coverage by 0.39%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage   89.76%   89.36%   -0.40%     
==========================================
  Files          45       47       +2     
  Lines        6919     7206     +287     
==========================================
+ Hits         6211     6440     +229     
- Misses        708      766      +58     
Flag Coverage Δ
python 80.64% <ø> (ø)
rust 89.48% <75.00%> (-0.41%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pineappl/src/grid.rs 87.78% <75.00%> (-0.16%) :arrow_down:
pineappl_cli/src/import.rs 85.33% <0.00%> (-8.00%) :arrow_down:
pineappl_cli/src/helpers.rs 87.42% <0.00%> (-3.91%) :arrow_down:
pineappl_cli/src/pull.rs 95.88% <0.00%> (-2.20%) :arrow_down:
pineappl_cli/src/plot.rs 91.37% <0.00%> (-1.38%) :arrow_down:
pineappl_cli/src/import/fastnlo.rs 86.13% <0.00%> (-0.27%) :arrow_down:
pineappl/src/import_only_subgrid.rs 95.11% <0.00%> (-0.03%) :arrow_down:
pineappl_cli/src/import/fktable.rs 97.22% <0.00%> (ø)
pineappl_applgrid/src/lib.rs 83.87% <0.00%> (ø)
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

andreab1997 commented 1 year ago

During the debug of NNPDF/runcards#150 we realized there is a bug in PineAPPL convolute_eko: x2_grid was determined although it was never needed (because x2 is trivial)

This fix adds a protection to the generation same as there is already for reading:

https://github.com/N3PDF/pineappl/blob/04650d99dd52f7985ba4fb2a976980b2b91ce6e7/pineappl/src/grid.rs#L1632

the quickfix (applied in NNPDF/runcards@3a5b7c9 ) is to point the trivial point to an existing point in the x1 grid - this is done also for DIS (and everything derived from that), specificly x2=1, which, by chance, is in the x1 grid; that's also the reason why it was not discovered before

there might be an other bug related: @scarlehoff @andreab1997 I couldn't trigger again the core dump - can you remind me, what do I need to do?

You just need to compute the fks for the integrability grid. It does not crash during the eko calculation.

felixhekhorn commented 1 year ago

You just need to compute the fks for the integrability grid. It does not crash during the eko calculation.

this is not what I meant - the error (concerning PineAPPL and not pineko ) in the fk generation was a "reached 'unreachable' code" (or similar) which is circumvented with this PR (it was the unreachable!() right there);

instead we had a specific ... core dump ... for a specific pineappl CLI call, which I can't remember which it was ... (I think it was convolute, but I can't reproduce the failing PineAPPL file )

scarlehoff commented 1 year ago

instead we had a specific ... core dump ... for a specific pineappl CLI call, which I can't remember which it was ... (I think it was convolute, but I can't reproduce the failing PineAPPL file )

It was convolute and it was a grid generated with this first commit https://github.com/NNPDF/runcards/pull/150/commits/de4605a73b54c6b6bf266a5fa7ff82c9132120ec however if it is not reproduced it is a good thing.

(I was changing the runcard trying to make it work so that grid in particular might have been fundamentally broken, it might be one of the first one I sent you but boh)

felixhekhorn commented 1 year ago

Actually - also all the grids in https://github.com/NNPDF/runcards/pull/132 suffer from this bug because the xgrid does not contain the trivial x2 point at x2=1

giacomomagni commented 1 year ago

Hi, is it possible to merge this PR? I need this fix to recompute some positivity fk tables at N3LO.

cschwan commented 1 year ago

I'm happy to merge it. @felixhekhorn is there something else needed here?

felixhekhorn commented 1 year ago

we can merge, I think (if ever I find the other bug, I'll open a new PR)

andreab1997 commented 1 year ago

It seems that this bug is also affecting the NLO positivities. I will wait this to be merged to compute them :)

felixhekhorn commented 1 year ago

To make life for @giacomomagni and @andreab1997 easier: can we also make a release?

giacomomagni commented 1 year ago

That will be really appreciated :). And thanks for solving the issue quickly

cschwan commented 1 year ago

Here you go: https://github.com/N3PDF/pineappl/releases/tag/v0.5.5. All the Python and Rust packages are also released, the conda package releases itself automatically as well, but it usually takes a bit longer.