Add merge to Python API

[alecandido]

Since Grid is now Clone, we can deprecate PyGrid::merge_from_file from the Python API and replace with just PyGrid::merge.

[cschwan]

Is Clone automatically propagated to Python by PyO3?

[alecandido]

Not sure about what do you mean, but consider that:

• to the wrapper, I added Clone myself
  https://github.com/NNPDF/pineappl/blob/d819050890c47426bc7fb5bb3f7ab69ced1c3ac8/pineappl_py/src/grid.rs#L103C1-L103C1
• in the pure Python wrapper, at that point all the Rust side is consumed, so there is no Clone any longer, because they are just Python objects

[felixhekhorn]

Are we missing something else then a unit test here? or why is this draft mode?

[cschwan]

A test would be nice, but I'm also OK merging it if someone tests it and reports success.

[alecandido]

I was waiting to write the test, that's why is still draft :)

[alecandido]

@cschwan I added a stub of the test for Grid.merge, but it's currently broken.

I didn't dig too much myself, but if you could take a look, maybe you could guess immediately what I'm doing wrong.

Moreover, the test is pretty basic, since I'm just counting the number of bins (assuming that merge is possibly extending those). Do you have ideas for a better one?

[cschwan]

I think you have non-overlapping bin limits, try changing the limits of the second grid.

[alecandido]

I tried to change, but it is still not working: I expect to add new bins into the merged grid, but I always find the same ones.

Unfortunately, there is little documentation about Grid::merge:

pineappl/pineappl/src/grid.rs

Lines 866 to 875 in c756b4c

	/// Merges the non-empty `Subgrid`s contained in `other` into `self`.
	///
	/// # Errors
	///
	/// If the bin limits of `self` and `other` are different and if the bin limits of `other` can
	/// not be merged with `self` an error is returned.
	///
	/// # Panics
	///
	/// TODO

So, if this becomes very relevant, I will dig into the code. But remaining at a superficial level (without exploring PineAPPL itself), I'm not able to tell if there is a bug in the (minimal) Python layer, or I'm misunderstanding Grid::merge usage.

[cschwan]

That seems to be the first problem:

tests/test_grid.py:23: in fake_grid
    g = pineappl.grid.Grid.create(lumis, orders, bin_limits, subgrid_params)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'pineappl.grid.Grid'>
lumi = [<builtins.PyLumiEntry object at 0x7ff7ff8f62d0>]
orders = [<builtins.PyOrder object at 0x7ff7ff8f63f0>]
bin_limits = array(None, dtype=object)
subgrid_params = <pineappl.subgrid.SubgridParams object at 0x7ff7ff42b6d0>

    @classmethod
    def create(cls, lumi, orders, bin_limits, subgrid_params):
        """Create a grid object from its ingredients.
    
        Parameters
        ---------
            lumi : list(LumiEntry)
                List of active luminosities
            orders: list(Order)
                List of available orders
            bin_limits: sequence(float)
                Bin limits
            subgrid_params : SubgridParams
                subgrid parameters
        """
        lumi = [lentry.raw for lentry in lumi]
        orders = [o.raw for o in orders]
>       return cls(PyGrid(lumi, orders, np.array(bin_limits), subgrid_params.raw))
E       TypeError: argument 'bin_limits': type mismatch:
E        from=object, to=float64

pineappl/grid.py:84: TypeError

If I read this error message properly, than the parameter bin_limits of the Grid constructor is an array of objects:

bin_limits = array(None, dtype=object)

Are you missing the .0 when creating the array?

[alecandido]

Thanks for the help, but unfortunately you're looking at the wrong example (not sure why that one, i.e. test_fill_all, is now failing, if not failing on master).

The one I'm now developing is failing here:
https://github.com/NNPDF/pineappl/actions/runs/6625861875/job/17997650555?pr=247#step:4:984
and the error is the following:

>       assert g.bins() == 4
E       assert 2 == 4
E        +  where 2 = <built-in method bins of builtins.PyGrid object at 0x5574f0dfda70>()
E        +    where <built-in method bins of builtins.PyGrid object at 0x5574f0dfda70> = <pineappl.grid.Grid object at 0x7ff7ff463b90>.bins

[alecandido]

Actually, I even know where is the problem you were looking: it is now taking the default value, i.e. None, I will fix it immediately.

[alecandido]

Actually, the two problems were somehow connected: fixing ones I've been able to fix the other.

However, the bins should be either consecutive (and so non-overlapping) or coinciding (inferred by trials), but this behavior is documented only by the errors I got.

In any case: if you have no better test in mind, I'm ready to merge.

[cschwan]

I'm happy about this, let's merge it!

[alecandido]

#245 (comment)