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

`Grid::axes` is too restrictive #197

Closed scarlehoff closed 1 year ago

scarlehoff commented 1 year ago

I'm trying to merge two grids. The merge seems to be successful and the information seems not to be lost (since a convolute after the merge does show the merged grid to be the sum of the other two) however the .axes() method stops working.

Here's the snippet of code that produces the error:

#!/usr/bin/env python

import numpy as np
from pineappl.grid import Grid
from pineappl.bin import BinRemapper

grid_1 = "./grid1.pineappl.lz4"
grid_2 = "./grid2.pineappl.lz4"
g2_rb = "./grid2_rebin.pineappl.lz4"
gout = "./gout.pineappl.lz4"

main_grid = Grid.read(grid_1)

# Rebin because I don't care about the observable itself
n = len(main_grid.bin_left(0))
rebin = BinRemapper(np.ones(n), [(i, i) for i in range(n)])
main_grid.set_remapper(rebin)

# Rebin also the secondary grid...
second_grid = Grid.read(grid_2)
second_grid.set_remapper(rebin)
second_grid.write(g2_rb)

# Now merge the rebinned grid into the main grid
main_grid.merge_from_file(g2_rb)

# And write out the final grid
main_grid.write(gout)

# At this point pineappl convolute gout ... does produce a result which is grid1 + grid2
# however...
final_grid = Grid.read(gout)
final_grid.axes() # EXCEPTION

The error is:

Traceback (most recent call last):
    final_grid.axes() # EXCEPTION
pyo3_runtime.PanicException: called `Option::unwrap()` on a `None` value

I've attached the grid1 and grid2 files to this issue.

grids1_and_2.zip

alecandido commented 1 year ago

That's a wonderful instance of our marvelous error management...

In principle, @cschwan already started addressing the problem with https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/Cargo.toml#L26 and https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/Cargo.toml#L29 but we need also to propagate in the Python interface, and it takes a little more

scarlehoff commented 1 year ago

What do you mean? You mean the error should have not happened? Or that the error is somewhere else?

I'd like a solution to the problem, not a better error necessarily...

alecandido commented 1 year ago

A better error would make it simpler to know where the error is generated.

What I can tell you at the moment is that one of these unwrap has been unexpectedly called on an error: https://github.com/search?q=repo%3ANNPDF%2Fpineappl%20unwrap&type=code Now starts the debugging game.

cschwan commented 1 year ago

@scarlehoff can you please post the error message again, but adding the environment variable export RUST_BACKTRACE=1 before? This should print the offending line number.

My guess is that you merge two grids, and the result is a Grid that can't be evolved.

scarlehoff commented 1 year ago

My guess is that you merge two grids, and the result is a Grid that can't be evolved

Why? If the Q/x information is known...

I've used export RUST_BACKTRACE=full because 1 didn't change the message.

python snippet.py 
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', src/grid.rs:319:30
stack backtrace:
   0:     0x7fdfa976a54d - <unknown>
   1:     0x7fdfa97896dc - <unknown>
   2:     0x7fdfa9768601 - <unknown>
   3:     0x7fdfa976bc25 - <unknown>
   4:     0x7fdfa976b8d9 - <unknown>
   5:     0x7fdfa976c172 - <unknown>
   6:     0x7fdfa976c019 - <unknown>
   7:     0x7fdfa976aa04 - <unknown>
   8:     0x7fdfa976bd89 - <unknown>
   9:     0x7fdfa96478b3 - <unknown>
  10:     0x7fdfa964777d - <unknown>
  11:     0x7fdfa967fb77 - <unknown>
  12:     0x7fdfa965c2b6 - <unknown>
  13:     0x7fdfa968385a - <unknown>
  14:           0x53c744 - <unknown>
  15:           0x5b1b93 - _PyEval_EvalFrameDefault
  16:           0x5ac171 - <unknown>
  17:           0x6e074f - PyEval_EvalCode
  18:           0x6c4b07 - <unknown>
  19:           0x6c4b90 - <unknown>
  20:           0x6c4d06 - <unknown>
  21:           0x6c9b14 - _PyRun_SimpleFileObject
  22:           0x6c9be7 - _PyRun_AnyFileObject
  23:           0x70d24d - Py_RunMain
  24:           0x70d42d - Py_BytesMain
  25:     0x7fe04b1ab083 - __libc_start_main
                               at /build/glibc-SzIz7B/glibc-2.31/csu/../csu/libc-start.c:308:16
  26:           0x63742e - _start
  27:                0x0 - <unknown>
Traceback (most recent call last):
  File "/home/juacrumar/NNPDF/pinefarm/results_DYE_NNLO/pinetest/snippet.py", line 33, in <module>
    final_grid.axes() # EXCEPTION
pyo3_runtime.PanicException: called `Option::unwrap()` on a `None` value
alecandido commented 1 year ago
src/grid.rs:319:30

This is the useful information :)

But I thought we were stripping symbols from the binary, I did not expect that information to be available.

alecandido commented 1 year ago

I guess we are not propagating metadata in the merge: https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L319

However, this line seems the wrong one, maybe I need to have look in the other src/grid.rs.

cschwan commented 1 year ago

It's a problem in pineappl_py/src/grid.rs, and there the message unfortunately doesn't help.

felixhekhorn commented 1 year ago

The problem is one of the following: https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1317 https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1321 https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1330 https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1340

alecandido commented 1 year ago

Remove my previous comment, the source it is obviously this: https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl_py/src/grid.rs#L319

We are failing one of these checks: https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1321-L1349

felixhekhorn commented 1 year ago

And https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1321 it is

$ pineappl subgrids --muf2 grid1.pineappl.lz4 | head
o b l   muf2
-+-+--+------
0 0 0  23.613
0 1 12 28.820
0 2 24 35.207
0 3 36 42.739
0 4 48 51.418
0 5 60 62.717
$ pineappl subgrids --muf2 grid2.pineappl.lz4 | head
o b l   muf2
-+-+--+------
0 0 0  25.213
0 1 12 30.641
0 2 24 37.470
0 3 36 45.815
0 4 48 54.908
0 5 60 66.799
scarlehoff commented 1 year ago

// the renormalization and factorization grid must be the same

Ah, so it should not have allowed me to merge in the first place. That's the actual problem.

(however, the results seem to correct, which means that somehow the muf2 information is not lost?)

felixhekhorn commented 1 year ago

(@cschwan is this panick a problem?

$ pineappl subgrids --muf2 grid2.pineappl.lz4 | head
o b l   muf2
-+-+--+------
[...]
1 0 0  25.213
1 0 1  25.213
thread 'main' panicked at 'Cannot print table to standard output : Broken pipe (os error 32)', /home/felix/.cargo/registry/src/github.com-1ecc6299db9ec823/prettytable-rs-0.8.0/src/lib.rs:194:23

)

felixhekhorn commented 1 year ago

// the renormalization and factorization grid must be the same

Ah, so it should not have allowed me to merge in the first place. That's the actual problem.

(however, the results seem to correct, which means that somehow the muf2 information is not lost?)

Mmm it seems a valid grid ... is axes() used for things other then FK tables? is it possible to create a grid which can not be turned into FK?

cschwan commented 1 year ago

(@cschwan is this panick a problem?

$ pineappl subgrids --muf2 grid2.pineappl.lz4 | head
o b l   muf2
-+-+--+------
[...]
1 0 0  25.213
1 0 1  25.213
thread 'main' panicked at 'Cannot print table to standard output : Broken pipe (os error 32)', /home/felix/.cargo/registry/src/github.com-1ecc6299db9ec823/prettytable-rs-0.8.0/src/lib.rs:194:23

)

This is an unrelated problem, head probably kills the pipe before pineappl has finished writing.

cschwan commented 1 year ago

Mmm it seems a valid grid ... is axes() used for things other then FK tables? is it possible to create a grid which can not be turned into FK?

It's possible to create a Grid that can't be evolved and axes was used to check exactly if it can. In this instance it says it can't be done.

scarlehoff commented 1 year ago

Because evolution needs the muF for a given observable to be constant?

cschwan commented 1 year ago

And

https://github.com/NNPDF/pineappl/blob/0fd43082d76df381b6c7912fe764af731aa0a17d/pineappl/src/grid.rs#L1321 it is

$ pineappl subgrids --muf2 grid1.pineappl.lz4 | head
o b l   muf2
-+-+--+------
0 0 0  23.613
0 1 12 28.820
0 2 24 35.207
0 3 36 42.739
0 4 48 51.418
0 5 60 62.717
$ pineappl subgrids --muf2 grid2.pineappl.lz4 | head
o b l   muf2
-+-+--+------
0 0 0  25.213
0 1 12 30.641
0 2 24 37.470
0 3 36 45.815
0 4 48 54.908
0 5 60 66.799

This I believe is a problem. When we implemented Grid::convolute_eko we said that, within a single order and bin, all luminosities must have the same factorization (and renormalization) scales. We should rethink whether we really want that restriction.

For Drell-Yan @ LO, for instance, one concern was that you could have up-anti-up living at muf = 80 GeV and down-anti-down living at muf = 90 GeV and that the application of EKOs, which mix the flavours, would lead to errors. A similar consideration holds for different x-grids. @felixhekhorn @AleCandido you should be able to answer that.

cschwan commented 1 year ago

One way to think about this is: if we 1) first evolve the grids and then merge them, do we get the same results as in the situation in which we 2) first merge then and then evolve them?

alecandido commented 1 year ago

You need to have all flavors available at all scales, because EKO mixes them. Or at least you need the full singlet and gluon to be available together, since they are coupled (in flavor basis this means everything).

So, the constraint is correct :)

cschwan commented 1 year ago

One way to think about this is: if we 1) first evolve the grids and then merge them, do we get the same results as in the situation in which we 2) first merge then and then evolve them?

To repeat @AleCandido's statement on Telegram:

PDF BC * FkTable = PDF BC * (EKO * grid) = PDF BC * EKO * grid = (PDF BC * EKO) * grid = PDF * grid

In words: the evolution is associative, meaning that we should assume 'missing' pieces in the Grid to be simply zero. If pieces are truly missing and the Grid is wrong, the evolution isn't the place to capture this mistake.

cschwan commented 1 year ago

I added a new a function Grid::evolve_info in commit 410e13a4e09eb3c085c9960905869b7c114c7a66 that doesn't impose any restrictions and will always successfully return the information needed to generate the corresponding EKO.

We'll need a Python interface to it and pineko will have to use that method instead of axes().

alecandido commented 1 year ago

I will provide it. As usual, it should take little :)

alecandido commented 1 year ago

The Python bindings for Grid::evolve_info are introduced in 75e0bd6805dcf1d52b30f39b984ec698d74ca8a0.

I have been a bit inconsistent with Grid::axes, and provided getters (i.e. python class properties) to have an interface closer to Rust one.

cschwan commented 1 year ago

Perfect :+1: !

cschwan commented 1 year ago

I believe that this issue has been fixed whenever we've verified that replacing Grid::axes in Pineko with Grid::evolve_info works.

cschwan commented 1 year ago

I'm closing this as the remaining problems must be solved in Pineko: https://github.com/NNPDF/pineko/issues/72.