Proposed changes to the behaviour of the PDF class.

[scarlehoff]

This is a collection of issues with the current implementation of the PDF class.

This issue will be organized as:

• Problem
• Examples
• Solution

Todo:

• Error type: the nnpdf_error attribute
• Loaded PDF class
• PDF .info file
• Closure test references

Error Type

Half of the issues with the PDF class is the management of the error type.

The nnpdf_error attribute:

nnpdf/validphys2/src/validphys/core.py

Line 180 in eeae19b

def nnpdf_error(self):

I will remove this attribute and all references to it. If something is accessing this attribute then it is a libNNPDF thing and should be removed. Instead (as was suggested) any behavior that depends on the error type of the PDF should be interfaced through its associated stats_class.

Actual PDF

Currently load loads a libNNPDF PDF set.

nnpdf/validphys2/src/validphys/core.py

Line 162 in eeae19b

def load(self):

It will be changed to a python wrapper to the lhapdf python interface. It makes sense to do it this way because it allows for certain optimization (such as grid_values).

Attributes

Currently we look inside the info file of the PDF by using hasattr, which is confusing and hides what's actually happening.

nnpdf/validphys2/src/validphys/core.py

Line 101 in eeae19b

def __getattr__(self, attr):

nnpdf/validphys2/src/validphys/core.py

Line 149 in eeae19b

if hasattr(self, "ErrorConfLevel"):

I will change it to something more explicit.

Reference to libNNPDF PDF

There are two instances where the libNNPDF PDF is used, both related to closure tests:

nnpdf/validphys2/src/validphys/closuretest/multiclosure.py

Line 36 in d68954e

def internal_multiclosure_dataset_loader(

nnpdf/validphys2/src/validphys/filters.py

Line 180 in eeae19b

loaded_data.MakeClosure(fakeset, fakenoise)

Of course, some of the issues highlighted here might have reasons behind them to which I'm not privy. I might also be missing other points that could be removed and that I'm overlooking.

EDIT: Change the places in which libNNPDF is necessary.

[Zaharid]

I agree with everything, with perhaps some reservation in regards to the last point, the t0 stuff, or the order in which to do various things. I think we could have t0 predictions and t0 covmat in pure python without the need to have to retrofit the cpp functionality in the new pdf, provided the t0 stuff is taken care of first. I suppose it wouldn't matter very much if the cpp interface could be removed easily but it might be a bit cleaner? @siranipour do you agree?

[scarlehoff]

The closure tests also need a libNNPDF PDF

nnpdf/validphys2/src/validphys/filters.py

Line 180 in eeae19b

loaded_data.MakeClosure(fakeset, fakenoise)

[siranipour]

There was a discussion regarding the t0 covmats here. I'm still a tad confused about this point. For example: we have this test that compares a pure python t0 covmat with the C++ t0 covmat:

nnpdf/validphys2/src/validphys/tests/test_covmats.py

Line 136 in eeae19b

def test_python_t0_covmat_matches_cpp(

Now I agree that

nnpdf/validphys2/src/validphys/config.py

Line 700 in eeae19b

def produce_covariance_matrix(self, use_pdferr: bool = False):

and

nnpdf/validphys2/src/validphys/config.py

Line 712 in eeae19b

def produce_dataset_inputs_covariance_matrix(self, use_pdferr: bool = False):

still dispatch the old C++ covariance matrix code, but in principle it should be easy to simply swap these out for the Python ones instead no?

In particular, notice that the python t0 covmat code does not use the libNNPDF PDF class, in particular the line here

nnpdf/validphys2/src/validphys/covmats.py

Line 319 in d68954e

_list_of_central_values=dataset_inputs_t0_predictions

is performing the convolution also purely in python. Now, I agree the PDF used in the convolution is C++ spec but surely this is sufficiently well contained that it doesn't directly impact the covmat code?

Again, apologies if I'm missing an obvious point, and for what it's worth I think the validphys.covmats module can do with some cleaning, because there is a lot of code that should have been removed long ago, namely the C++ ones (I suppose we kept them for backwards compatibility).

I do, however, agree with the remainder of the points made, and thanks @scarlehoff for a nicely formatted summary of the issues.

[scarlehoff]

it should be easy to simply swap these out for the Python ones instead no?

I don't know. When destroying C++ I would like to avoid removing whole chunks of code just because it is inconvenient, specially if it is being used.

Actually if those two production rules can just be swapped by the python ones I'd appreciate if someone does it so it's one less thing I have to worry about.

Then there's the MakeClosure stuff which will still be a problem.

[scarlehoff]

Changed the initial comment to remove the two cases that have been removed thanks to #1477

[scarlehoff]

@siranipour @Zaharid do you have any feeling on how difficult would it be to change the two places in which we are still using PDFs through libNNPDF? They are both closure-test related. The first one seems -naively- easy to bypass, the second I don't really know.

nnpdf/validphys2/src/validphys/closuretest/multiclosure.py

Line 96 in d68954e

loaded_data.SetT0(multiclosure_underlyinglaw.load_t0())

nnpdf/validphys2/src/validphys/filters.py

Line 180 in eeae19b

loaded_data.MakeClosure(fakeset, fakenoise)

[Zaharid]

@siranipour do we have the capability to to generate closure test replicas with all the options from Python?

[siranipour]

Unfortunately, I don't think we do yet... This was something that we failed to remember when implementing pseudodata.py at the time

[Zaharid]

I suppose it it a bit of a priority now, as it is standing in the way of making progress here...