Skip to content

convolute_eko: x2_grid was determined though not needed#167

Merged
cschwan merged 2 commits into
masterfrom
bugfix-convolute_eko-skip-x2_grid
Aug 25, 2022
Merged

convolute_eko: x2_grid was determined though not needed#167
cschwan merged 2 commits into
masterfrom
bugfix-convolute_eko-skip-x2_grid

Conversation

@felixhekhorn
Copy link
Copy Markdown
Contributor

During the debug of NNPDF/pinecards#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/pinecards@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?

@felixhekhorn felixhekhorn added the bug Something isn't working label Aug 18, 2022
@felixhekhorn felixhekhorn requested a review from cschwan August 18, 2022 13:29
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 18, 2022

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%) ⬇️

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%) ⬇️
pineappl_cli/src/import.rs 85.33% <0.00%> (-8.00%) ⬇️
pineappl_cli/src/helpers.rs 87.42% <0.00%> (-3.91%) ⬇️
pineappl_cli/src/pull.rs 95.88% <0.00%> (-2.20%) ⬇️
pineappl_cli/src/plot.rs 91.37% <0.00%> (-1.38%) ⬇️
pineappl_cli/src/import/fastnlo.rs 86.13% <0.00%> (-0.27%) ⬇️
pineappl/src/import_only_subgrid.rs 95.11% <0.00%> (-0.03%) ⬇️
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
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

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 NNPDF/pinecards@de4605a 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
Copy link
Copy Markdown
Contributor Author

Actually - also all the grids in NNPDF/pinecards#132 suffer from this bug because the xgrid does not contain the trivial x2 point at x2=1

@giacomomagni
Copy link
Copy Markdown

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

@cschwan
Copy link
Copy Markdown
Contributor

cschwan commented Aug 25, 2022

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

@felixhekhorn
Copy link
Copy Markdown
Contributor Author

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

Comment thread pineappl/src/grid.rs Outdated
Comment thread pineappl/src/grid.rs Outdated
@andreab1997
Copy link
Copy Markdown
Collaborator

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

@cschwan cschwan merged commit 8ced9d0 into master Aug 25, 2022
@cschwan cschwan deleted the bugfix-convolute_eko-skip-x2_grid branch August 25, 2022 11:29
@felixhekhorn
Copy link
Copy Markdown
Contributor Author

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

@giacomomagni
Copy link
Copy Markdown

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

@cschwan
Copy link
Copy Markdown
Contributor

cschwan commented Aug 25, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants