Use python convolutions also for positivity predictions

[scarlehoff]

For this I had to introduce a new attribute and an if condition in the predictions module. Another option is to create a fake op and fake cuts in the positivity spec class I guess.

The first commit includes only the if condition and the extra attribute to create a record of the cpp and the python version giving the same result, I'll push the change to PositivityResult later if that passes.

[Zaharid]

Could we make positivity be more like datasets without data instead? AFAICT a minimal step would be just declaring that they have OP=NULL and empty cuts for the moment, which would achieve a similar effect.

[scarlehoff]

which would achieve a similar effect.

The problem is the cuts (and that they don't have fkspecs but fkspec but that's minimal ofc).

[Zaharid]

Can we make the cuts empty?

[scarlehoff]

If #1506 goes through the same trick can be used here, yes (or whatever trick is used there after review)

[scarlehoff]

Marking this as draft because I would like to do #1506 first and would like to also add tests for the plots to ensure that bugs like the one shown in #1504 are avoided

[github-actions]

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

• Fit Name: NNBOT-597e3fa03-2022-02-01
• Fit Report: https://vp.nnpdf.science/4A-pefmFRMCh4W4A41R7-g==
• Fit Data: https://data.nnpdf.science/fits/NNBOT-597e3fa03-2022-02-01.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

[Zaharid]

Why?

[scarlehoff]

I don't know why. But it seems to be the case (it is even called fkspec instead of fkspecs for positivity)

[scarlehoff]

It was actually quite easy to make the Positivity behave as a normal Dataset, the only actual problems were due to things like fkspec vs fkspecs or commondata vs commondataspec but they seem to be perfectly compatible.

This allows for the removal of some of the specific functions for positivity (that were not used anywhere) since the generic ones can be used as well. One can go even a step forward and allow for operations on positivity but I haven't entered there.

[Zaharid]

Thinking in terms of #1500, a positivity should have everything that a theory prediction has (i.e. OP, a list of fktables) plus everything commondata has except for the actual data (same metadata, kinematics). cc @enocera

For now, it seems like a good idea to make it inherit from DatasetSpec as done here. But I'd get rid of the various gratuitous differences such as allowing only one fkspec, as these don't buy us very much.

[scarlehoff]

But I'd get rid of the various gratuitous differences such as allowing only one fkspec, as these don't buy us very much.

Sadly those come from libNNPDF, but I agree, there should be no difference other than the actual data.

[Zaharid]

But I'd get rid of the various gratuitous differences such as allowing only one fkspec, as these don't buy us very much.

Sadly those come from libNNPDF, but I agree, there should be no difference other than the actual data.

But isn't the whole point of the exercise that we don't care any more?

[scarlehoff]

But isn't the whole point of the exercise that we don't care any more?

Yes, we remove the things where libNNPDF is used little by little until it can be completely extirpated. But right now this ends whenever we have a libNNPDF class for which we have no substitute and, for positivity, that class happens to like single fktables:

nnpdf/validphys2/src/validphys/core.py

Line 563 in ab7f053

return PositivitySet(cd, fk, self.maxlambda)

The same is true for the actual data, of course, but that accepts an fkset instead:

nnpdf/validphys2/src/validphys/core.py

Line 501 in ab7f053

data = DataSet(cd, fkset, self.weight)

which is some combination of fktables:

nnpdf/validphys2/src/validphys/core.py

Line 499 in ab7f053

fkset = FKSet(FKSet.parseOperator(self.op), fktables)

(also coming from libNNPDF)

Eventually both libNNPDF::PositivitySet and libNNPDF::DataSet will be replaced for inheritances of GenericDataHolder or whatever which won't have such inconsistencies in their signatures.

[Zaharid]

But why do we need to call the cpp class from python anymore? We don't have many whaky uses of it and seems to me it can be all replaced in one PR.

[Zaharid]

Do we even load it anywhere but here?

src/validphys/results.py
720:def positivity_predictions_data_result(pdf, posdataset):
722:    return PositivityResult.from_convolution(pdf, posdataset)

[scarlehoff]

I don't know, but I'd rather wait until we have a substitute for the DataSet to create a PositivitySet that inherits from it rather than doing the PositivityOne first...

[Zaharid]

We can of course do it in two time, but seems to me having to change the interface negates many benefits of that. OTOH having a fully python positivity might help surface issues with the pure python approach.

[scarlehoff]

Given that I broke completely positivity in #1504 and nothing broke until I looked at the comparefits I very much doubt that we'll see anything surface...

[Zaharid]

Given that I broke completely positivity in #1504 and nothing broke until I looked at the comparefits I very much doubt that we'll see anything surface...

I think that can be used as argument either way :)

But as said I'd favor forgetting about loading positivity sets via libnnpdf in vp, so we are more free to design the interface we like, in particular removing a couple of constraints from the current pr.

[scarlehoff]

I think that can be used as argument either way :)

I don't see how.

Anyway, we do load positivity apparently exactly like we do with the datasets and given that they should be basically the same thing they probably should be changed at the same time: https://github.com/NNPDF/nnpdf/blob/master/validphys2/src/validphys/n3fit_data_utils.py and I would strongly prefer to do the datasets, which are the more general object, first.

Otherwise I will be creating some PositivitySetSpec that doesn't include a Cuts attribute and that uses commondataspec instead of commondata and that is completely incompatible with what should be the generic DataSetSpec.

[scarlehoff]

Anyway, given that this one depends on #1506 we should finish that before we discuss on how to improve what's not yet done here x)

[scarlehoff]

I should stop solving conflicts with the github editor because I do more harm than good...

[github-actions]

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

• Fit Name: NNBOT-15b1e9e07-2022-02-09
• Fit Report: https://vp.nnpdf.science/3KVGP2GiQAKlNzWqAuM7rA==
• Fit Data: https://data.nnpdf.science/fits/NNBOT-15b1e9e07-2022-02-09.tar.gz

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

[Zaharid]

As discussed I get the impression that the forced c++ compatibility can be dealt away swiftly, in a different PR. I think that should be done before the new commondata format is introduced as that should not be a prerequisite for any other work. But what is in here should be fine for now.