Accommodate for two different EKOs in Evolution

[Radonirinaunimi]

Addresses #288. So far, this is just a bare brute-force implementation to test if it conceptually works.

Remaining to do:

• add some unit tests
• add checks that verify the compatibility between the initial states and the EKOs (Moved to Pineko)
• Fix the CLI to accept different EKOs
• deprecate Grid::evolve in favor of Grid::evolve_with_slice_iter
• simplify the passing of EKOs
• make sure changes from Allow to specify multiple convolution functions with CLI #293 are taken into account: before merging this branch into master
  • after having merged Allow to specify multiple convolution functions with CLI #293 into master
  • merge master into this branch
• remove the methods Grid::evolve2 and its Python wrapper; confirm that you don't use it in Pineko

[cschwan]

This goes in the right direction, but I'd like to generalize this even further for the cases where we have 3 or even arbitrarily many evolutions; let's design the interface with arbitrarily many evolutions/convolutions in mind, and then let's focus first on the important cases.

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs. Then we'll need - I suppose - a single object that returns EKOs for each Convolution type. This object should replace operator_a, operator_b and slice_info.

[Radonirinaunimi]

This goes in the right direction, but I'd like to generalize this even further for the cases where we have 3 or even arbitrarily many evolutions; let's design the interface with arbitrarily many evolutions/convolutions in mind, and then let's focus first on the important cases.

That's right! I absolutely agree with this.

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs. Then we'll need - I suppose - a single object that returns EKOs for each Convolution type. This object should replace operator_a, operator_b and slice_info.

Having a single object that replaces operator_a, operator_b, ..., operator_n would indeed be the best and cleanest solution (although I don't have a clear idea yet how that object should look like). I am currently still in the process of familiarizing myself with all the parts, but will propose something very soon xd.

[felixhekhorn]

I wonder if this feature is a priority? since fitting with several distributions is rather a mid- or long-term feature ... in the short term we want to close one convolution (with a fixed PDF), produce a new grid with only one open convolution and turn that into an FK table, wouldn't you say so?

[Radonirinaunimi]

I wonder if this feature is a priority? since fitting with several distributions is rather a mid- or long-term feature ... in the short term we want to close one convolution (with a fixed PDF), produce a new grid with only one open convolution and turn that into an FK table, wouldn't you say so?

That's absolutely true! But if this could be achieved with more or less the same efforts, that'll be a better no? But I agree that if this turns out to be more difficult, we could delay it for later.

[cschwan]

I believe a priority should be to have a stable interface, which can be stable for a long time. This means it should account for all the various EKOs possible, and then the most general implementation can take longer as I already said in #289 (comment).

Note that for now, we don't even use the new Grid::evolve_with_slice_iter in Pineko, which is already the third iteration of evolution in PineAPPL (first was Grid::convolute_eko, second was Grid::evolve).

[Radonirinaunimi]

Now that we have Grid::convolutions, we can use it to check which convolutions require different EKOs.

Thinking about this a bit, we can use Grid::convolutions to check if the grid requires different EKOs, but ultimately the different EKOs have to be passed as inputs from pineko (or from the CLI) - and this is really what should define the "EKO object".

So, I think the easiest and best solution would be to pass a list of EKOs to:

pineappl/pineappl/src/grid.rs

Lines 1400 to 1406 in 95b3bc3

	pub fn evolve(
	&self,
	operator_a: ArrayView5<f64>,
	operator_b: ArrayView5<f64>,
	info: &OperatorInfo,
	order_mask: &[bool],
	) -> Result<FkTable, GridError> {

in that we replace operator_ with list_operators, and then check with Grid::convolutions that the number and types of EKOs are consistent with the initial states.

In this way, we can arbitrarily pass different evolutions. Does this sound reasonable?

[felixhekhorn]

@cschwan original idea was based on an iterative approach: calling evolve several times with one operator each and closing one (multi-)index at a time - I think this might be conceptually easier then dealing with all convolutions at the same time. Ofc there can always be some sugar to do all in one go

[cschwan]

@cschwan original idea was based on an iterative approach: calling evolve several times with one operator each and closing one (multi-)index at a time - I think this might be conceptually easier then dealing with all convolutions at the same time. Ofc there can always be some sugar to do all in one go

Indeed, but using this approach it's easy to get it wrong, so I think I've changed my mind 😄.

[cschwan]

In this way, we can arbitrarily pass different evolutions. Does this sound reasonable?

That's what I had in mind, although I'm still unclear about the details.

[giacomomagni]

Hi all,
since now we made quite a few progress in NNPDF/pineko#181 and we are ready to support
pineappl==0.7.4 in Pineko, would it be possible to merge this PR and have a new micro release, so these changes
are more accessible.
In particular we will need to introduce FkTable.convolve_with_two also in n3fit.
Thanks!!

[Radonirinaunimi]

@cschwan, @felixhekhorn: After having looked a bit, I think generalizing to any arbitrary evolution is complicated and requires a lot of changes (because for example, most of the structs will have to be modified).

As far as this PR is concerned, this already achieves the initial intended purposes, and we already tested it in NNPDF/pineko#181. In this sense, at least, I think this can be reviewed. What do you think?

[cschwan]

@Radonirinaunimi I've implemented most of the necessary changes, in particular the Python binding for Grid::evolve_with_slice_iter2. In pineappl_py/tests/test_fk_table.py you can see an example how to use this function. Could you try to adapt Pineko to use this function?

[giacomomagni]

@Radonirinaunimi I've implemented most of the necessary changes, in particular the Python binding for Grid::evolve_with_slice_iter2. In pineappl_py/tests/test_fk_table.py you can see an example how to use this function. Could you try to adapt Pineko to use this function?

This seems to work NNPDF/pineko@d62dac4
thanks @cschwan

[Radonirinaunimi]

I finally had time to carefully look into this again. Thanks again the for changes @cschwan (and especially for the improvements), these look really good to me. I believe we've achieved what we ought to do in this PR, should this be merged now?

[cschwan]

We didn't manage to fully generalize the evolution interfaces, but that wasn't the intention of this pull request so that's OK. In the future, we'll have to change it again, but so is life.

@Radonirinaunimi we can almost merge, can you please take remove the methods Grid::evolve2 and its Python wrapper; confirm that you don't use it in Pineko.

[Radonirinaunimi]

We didn't manage to fully generalize the evolution interfaces, but that wasn't the intention of this pull request so that's OK. In the future, we'll have to change it again, but so is life.

@Radonirinaunimi we can almost merge, can you please take care of the following:

* [ ]  remove the methods `Grid::evolve2` and its Python wrapper; confirm that you don't use it in Pineko

Yes, I can do this.

[cschwan]

When you've removed the methods, you can merge this branch into master. Then I'm going to prepare a new release 0.8.1 that will have the new method.

[Radonirinaunimi]

When you've removed the methods, you can merge this branch into master. Then I'm going to prepare a new release 0.8.1 that will have the new method.

Perfect! Awesome.

The method has been removed, I will wait for the tests to succeed and then merge. Once v0.8.1 is available I will propagate it to nnpdf and pineko to make the CI working again.

Thanks again 🚀

[cschwan]

Great, thank you!