Skip to content

Swap libNNPDF LHAPDFSet for a python PDF object#1501

Merged
Zaharid merged 7 commits into
masterfrom
remove_libNNPDF_LHAPDFSet
Feb 9, 2022
Merged

Swap libNNPDF LHAPDFSet for a python PDF object#1501
Zaharid merged 7 commits into
masterfrom
remove_libNNPDF_LHAPDFSet

Conversation

@scarlehoff
Copy link
Copy Markdown
Member

@scarlehoff scarlehoff commented Jan 21, 2022

Final step of #1472, with the previous changes this is just a drop-in replacement. At least from the point of view of the tests nothing changes (the test will fail because the effective exponent for smallx for t3 changed).

The only thing missing from here in order to close fully #1472 is how to avoid having to run the convolution twice to get the central value for observable predictions (which is a factor of x2 every time one tries to get a prediction). See issue #1502 for more on this.

Note that a legacy_load is still left there because the filter.py uses a PDF for MakeClosure.

@scarlehoff scarlehoff marked this pull request as draft January 21, 2022 11:24
@scarlehoff
Copy link
Copy Markdown
Member Author

The test that fails (effective exponent) is due to how sensitive NNPDF to precision when the PDF is close to 0 which triggers this:

alphaGrid_values[alphaGrid_values == - np.inf] = np.nan # when PDF_i =0

In the test itself this has an effect only for T3 (alpha from -0.4 to -0.6) which is the only negative exponent that we have. fwiw I think this version is better than master in that the precision is a bit better (which in turns might mean a PDF that before was 0 now it's just 1e-8 or whatever).

@scarlehoff
Copy link
Copy Markdown
Member Author

scarlehoff commented Jan 28, 2022

For the reviewer, note that this is partial PR and that needs #1504 to fully work in a sensible manner (or in a way that I am comfortable with).
I think comparefits might fail with this PR.

#1504 might be too complicated to be done at the same time as this one so I would prefer if this is reviewed and merged as soon as possible tbh so at least this part is completed.

@scarlehoff scarlehoff requested a review from Zaharid January 28, 2022 10:58
@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Jan 28, 2022
@github-actions
Copy link
Copy Markdown

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 😉!

@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Feb 3, 2022

@scarlehoff Should I start with this one then?

Comment thread validphys2/src/validphys/lhapdfset.py Outdated
@scarlehoff
Copy link
Copy Markdown
Member Author

@scarlehoff Should I start with this one then?

Yes, this one should be ready.

Comment thread validphys2/src/validphys/lhapdfset.py Outdated
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Feb 3, 2022

@tgiani some help with the remaining cpp methods, particularly MakeClosure would be appreciated here. Should not be too hard to do in python with all the pseudodata machinery in place.

@scarlehoff
Copy link
Copy Markdown
Member Author

Note that the positivity one for instance won't be needed any longer (in the other PR that convolution is done with python)

return len(self.members)

@property
def members(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I take it changing this is the other PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You mean members being 101 instead of 100?

Comment thread validphys2/src/validphys/lhapdfset.py Outdated
@Zaharid
Copy link
Copy Markdown
Contributor

Zaharid commented Feb 3, 2022

This looks fine to me, given that it is not doing much dangerous stuff on its own.

@Zaharid Zaharid merged commit 8e81729 into master Feb 9, 2022
@Zaharid Zaharid deleted the remove_libNNPDF_LHAPDFSet branch February 9, 2022 15:33
@Zaharid Zaharid added the enhancement New feature or request label May 12, 2022
@github-actions
Copy link
Copy Markdown

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 😉!

@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

destroyingc++ enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants