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

Filling a grid with python results in always 0 due to `set_q2_min` not being available in python #164

Closed scarlehoff closed 1 year ago

scarlehoff commented 1 year ago

I don't understand why this would not work:

pdf = lhapdf.mkPDF("NNPDF40_nnlo_as_01180")
luminosities = [pineappl.lumi.LumiEntry([(1,2,1.0)])]
orders = [pineappl.grid.Order(0, 0, 0, 0)]
params = pineappl.subgrid.SubgridParams()
grid = pineappl.grid.Grid.create(luminosities, orders, [0.0, 1.0], params)
grid.fill(0.2, 0.2, 10, 0, 0.5, 0, 0.5)
res = grid.convolute_with_one(2212, pdf.xfxQ, pdf.alphasQ)

Could it be related to https://github.com/N3PDF/pineappl/pull/107#issuecomment-1035498740 ?

cschwan commented 1 year ago

You set $Q^2 = 10$, but the default minimum is $Q^2 = 100$. Raise this value and it'll be filled in.

scarlehoff commented 1 year ago

How do I change the minimum with python?

Also, it is a bit confusing because the following does fill the grid (so the minimum has an effect only on fill?):

subgrid = pineappl.import_only_subgrid.ImportOnlySubgridV1([[[0.5]]], [10.0], [0.2], [0.2])
grid.set_subgrid(0, 0, 0, subgrid)
cschwan commented 1 year ago

How do I change the minimum with python?

I'm not sure this method is available in Python, but try adding the following after creating params in your example at the top:

params.set_q2_min(1.0)

If it's not available we should add it.

Also, it is a bit confusing because the following does fill the grid (so the minimum has an effect only on fill?):

Exactly, only fill is affected (more accurately the subgrid parameters determine how the interpolation is done, which is fill's job).

scarlehoff commented 1 year ago

I'm not sure this method is available in Python

It is not.

I'll change the title of the issue to reflect the actual bug.

cschwan commented 1 year ago

@AleCandido could you please add the corresponding methods?

alecandido commented 1 year ago

Yes, I'll do. It should take little :)

alecandido commented 1 year ago

Ok, cab1505cb6233cb64caf7cecc54be2372fe1b0d0 should introduce all the methods to set $Q^2$ related parameters.

Unfortunately, it is hard to test, because we are lacking all the getters for SubgridParams. What I did is just running the snippets above (that I'm now going to include in the unit tests), and the results is consistent:

# [...]
grid.convolute_with_one(2212, pdf.xfxQ, pdf.alphasQ)
# array([0.])
params.set_q2_min(1.0)
grid = pineappl.grid.Grid.create(luminosities, orders, [0.0, 1.0], params)
grid.fill(0.2, 0.2, 10, 0, 0.5, 0, 0.5)
grid.convolute_with_one(2212, pdf.xfxQ, pdf.alphasQ)
# array([2.11054246])

Sorry to have taken, many other things happened in the meanwhile...

alecandido commented 1 year ago

If you confirm the above result, in principle we could even close this issue.

But: what about the getters?

What is worth is that there is also no getter for the subgrid_params attribute of the Grid object. So, given a Grid, you can not:

  1. inspect what parameters have been set
  2. change the parameters of the subgrid

While the second one is almost a feature I'd like to preserve, the first one is a shortcoming of the API. What should I expose more?

  1. getters for SubgridParams's attributes
  2. getter for subgrid_param attribute of Grid

Of course this can also be moved to a new issue.

alecandido commented 1 year ago

In the commit above I added also the unit test, so for me this can be closed @cschwan

What do we want to do about getters? @cschwan @scarlehoff @felixhekhorn

cschwan commented 1 year ago

We should also add getters, although I'm going to change this structure in the future to have public fields, because the getters and setters are all trivial. I'm not certain how this would be done in Python though.

alecandido commented 1 year ago

I believe they will propagate as attributes. Even though maybe is not completely trivial https://pyo3.rs/v0.17.1/class.html#object-properties.

Unless you're going to use pub(crate). In which case I believe is not going to happen, since pineappl_py is another crate, so I won't be able to access them.

cschwan commented 1 year ago

I'm thinking of pub, actually, and then we'd probably need https://pyo3.rs/v0.17.1/class.html#object-properties-using-getter-and-setter.

alecandido commented 1 year ago

That's for sure an option.

I would have liked more the more intrinsic #[pyo3(get, set)], but I believe that with our layout is rather complicated. We just have a wrapper, and I do not want to mirror the struct to a new one with the same fields...

cschwan commented 1 year ago

Exactly, that's the problem I see.

cschwan commented 1 year ago

Can this be considered solved?