Fix Python list arguments

[felixhekhorn]

I could not find back the comment of @cschwan where he was asking about why we're using sometimes Vec and sometime PyReadonlyArray and @Radonirinaunimi replied, that should not make a difference. The unit tests here prove it does make a difference (i.e. they will not pass without the changes in the interface) and the correct thing is to use Vec.

Else you get

        np.testing.assert_allclose(
>           g.convolve_with_one(2212, lambda pid, x, q2: 1, lambda q2: 1.0,bin_indices=[0]),
            [v],
        )
E       TypeError: argument 'bin_indices': 'list' object cannot be converted to 'PyArray<T, D>'

tests/test_grid.py:105: TypeError

and likewise wrapping in np.array does not work

        np.testing.assert_allclose(
>           g.convolve_with_one(2212, lambda pid, x, q2: 1, lambda q2: 1.0,bin_indices=np.array([0])),
            [v],
        )
E       TypeError: argument 'bin_indices': 'ndarray' object cannot be converted to 'PyArray<T, D>'

tests/test_grid.py:105: TypeError

however, this must have changed recently (e.g. in #302) because in v0.7.4 where I was hit by the problem I could still do

    order_mask = pineappl.grid.Order.create_mask(grid.orders(), 2 - 1 + pto_, 0, True)
    for lu, lab in enumerate(("gg", "qqbar", "gq")):
        lumi_mask = [False] * 6
        lumi_mask[lu] = True
        df[lab] = grid.convolute_with_one(
            2212,
            extra.masked_xfxQ2(central_pdf),
            central_pdf.alphasQ2,
            lumi_mask=lumi_mask,
            order_mask=order_mask,
        )

also note that in v0.7.4 the error is slightly different:

return self.raw.convolute_with_two(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: argument 'bin_indices': type mismatch:
 from=int64, to=uint64

do we want to keep fixing former version, or not? (else I can work around that - I was too lazy so far to upgrade to v0.8 as it is a major change and I know v0.9 will be as well)

[Radonirinaunimi]

I actually mentioned the opposite: https://github.com/NNPDF/pineappl/pull/302/files#r1709362111. Both are different with PyReadonlyArray being a "subset" of Vec. There is an exception though in which this is not true and that concerns the bool vs bp.bool_ types. Basically, if one passes a numpy array of booleans, Vec will fail. And this was the reason I did not yet change everything into Vec.

[felixhekhorn]

Mmm seems I misremembered ... so maybe we want to discuss the *_masks and bin_indices separately? In any case we have to support these arguments

PS: and the unit tests do, what a user would do, no?

[cschwan]

Replace <>.map_or(vec![], ...) with <>.unwrap_or(vec![]). That saves the second argument which is unnecessary now; it converts a Vec into the same Vec, which is essentially clone.

The more general question is: Shouldn't we replace all instances PyReadonlyArray1 with Vec?

[felixhekhorn]

Replace <>.map_or(vec![], ...) with <>.unwrap_or(vec![]). That saves the second argument which is unnecessary now; it converts a Vec into the same Vec, which is essentially clone.

done in 5117ea1 (note that I applied the same logic to xi as I believe it still holds)

The more general question is: Shouldn't we replace all instances PyReadonlyArray1 with Vec?

not sure - @Radonirinaunimi since you have already though about it: can you please give an advice? 😇

[cschwan]

[..] Basically, if one passes a numpy array of booleans, Vec will fail. And this was the reason I did not yet change everything into Vec.

Since we're going to make a 'major' release, I think we can make (more) breaking changes. What do you think, @Radonirinaunimi?

[cschwan]

@felixhekhorn sorry, but there's an even better variant: .unwrap_or_default(). This has the advantage that we don't create a Vec<_> when it's not needed.

[felixhekhorn]

@felixhekhorn sorry, but there's an even better variant: .unwrap_or_default(). This has the advantage that we don't create a Vec<_> when it's not needed.

done in 9a163bb

[Radonirinaunimi]

Apologies for having forgotten about this (got dumped into a mountain of things)!

After some thinking, yes, I do think that there is more benefit in having the input types to be uniform and general as much as possible. Especially since I don't think this would be much of a breaking change. Actually, as mentioned here, Vec will accept all lists/arrays/vectors except for order_mask which will have to be a list or a tuple.

So, we should go for this change, and at the same propagate it into the other functions.

Extra-note: Actually, I think that if we change the return type of the following function into Vec then pineko will still work out of the box.

pineappl/pineappl_py/src/boc.rs

Lines 129 to 144 in 7053cec

	pub fn create_mask<'py>(
	orders: Vec<PyRef<Self>>,
	max_as: u32,
	max_al: u32,
	logs: bool,
	py: Python<'py>,
	) -> Bound<'py, PyArray1<bool>> {
	Order::create_mask(
	&orders.iter().map(|o| o.order.clone()).collect::<Vec<_>>(),
	max_as,
	max_al,
	logs,
	)
	.into_pyarray_bound(py)
	}
	}