Support evolution basis FkTables

[alecandido]

We want to support the generation of FkTables in evolution basis, in order to get closer to the old FkTables.

Since the rotation is already available inside eko, the only thing missing was to allow for different input and output basis during convolution of pineappl grids.
(actually the only information relevant for the inner convolution is the length of the basis, that has to be the same for input and output, so the only thing left is to propagate the information on output basis to match it correctly with the PDF later on)

[codecov]

Codecov Report

Merging #75 (c422fb6) into master (02c4f45) will increase coverage by 0.68%.
The diff coverage is 60.46%.

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   70.35%   71.03%   +0.68%     
==========================================
  Files          12       13       +1     
  Lines        1845     2120     +275     
==========================================
+ Hits         1298     1506     +208     
- Misses        547      614      +67     

Impacted Files	Coverage Δ
pineappl/src/grid.rs	61.11% <60.46%> (+19.56%)	⬆️
pineappl/src/lumi.rs	16.66% <0.00%> (-83.34%)	⬇️
pineappl/src/pids.rs	0.00% <0.00%> (ø)
pineappl/src/empty_subgrid.rs	94.11% <0.00%> (+11.76%)	⬆️
pineappl/src/fk_table.rs	38.96% <0.00%> (+38.96%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02c4f45...c422fb6. Read the comment docs.

[cschwan]

@felixhekhorn + commit c42860c: Please don't do that, instead add metadata to the Grid that convolute_eko creates. The argument list of convolute_eko is already far too long.

[felixhekhorn]

@felixhekhorn + commit c42860c: Please don't do that, instead add metadata to the Grid that convolute_eko creates. The argument list of convolute_eko is already far too long.

We thought about it, but we do like the alternative even less:

• to my understanding FkTables should be immutable, i.e. read-only, because you're never supposed to change them once they are created (e.g. set_key_value should not be allowed)
• and @alecandido correctly said that we would also have a simply wrong FkTable in the middle ...

[cschwan]

@felixhekhorn So what we want is to add very specific metadata to the FkTable, which when it is missing or wrong is an error. In that case we better add new parameters to the arguments of convolute_eko. The advantage of that over a HashMap is that the metadata has to be given explicitly and is strongly typed.

We should probably pack all the arguments, except maybe the operator itself into a new struct to keep the number of the arguments small (that's independent of of the discussion above, but adding more arguments compounds the problem).

[alecandido]

@felixhekhorn So what we want is to add very specific metadata to the FkTable, which when it is missing or wrong is an error. In that case we better add new parameters to the arguments of convolute_eko. The advantage of that over a HashMap is that the metadata has to be given explicitly and is strongly typed.

We should probably pack all the arguments, except maybe the operator itself into a new struct to keep the number of the arguments small (that's independent of of the discussion above, but adding more arguments compounds the problem).

I agree with both.

Later on we should also improve a bit the convolute_eko function itself, since at the moment it's barely readable. I know it's a complex operation, but maybe we can refactor with some tinier functions to perform more atomic tasks (as I said, later on...).

[alecandido]

Let's recap what we need here to merge, not leave things open forever:

• convert most of the inputs of convolute_eko to a suitable struct (to be introduced)
• add tests to make codecov happy again

Is there anything else?

[alecandido]

I'm on my way to remove the additional_metadata, but since I had to update EkoInfo (that now has a different meaning), I took the chance to reshape it in a more consistent way.

Can you review and make your comments? @cschwan

[cschwan]

@alecandido thanks, the EkoInfo struct was really wrongly named, but now it correctly reflects the fact that it's information from EKO.

[alecandido]

Yes, on the other hand I'm not 100% convinced about GridAxes, because at some point (hope never) we might need to pass EKO more information, that is not only the axes.

Nevertheless, we can say that for the time being is fine, and maybe it'll be fine forever.

[cschwan]

@alecandido

• it looks OK for the time being - and as you said we'll simply improve it whenever we know how.
• in commit 45963f4 I improved the docstrings a bit.
• do you really need the grid_axes member in EkoInfo? I think we can simply use let grid_axes = self.axes()?;, which we already use in any case:

https://github.com/N3PDF/pineappl/blob/45963f45d1b1bfb37476d902f201fbdcd7597038/pineappl/src/grid.rs#L1262-L1263

[alecandido]

• do you really need the grid_axes member in EkoInfo? I think we can simply use let grid_axes = self.axes()?;, which we already use in any case:

https://github.com/N3PDF/pineappl/blob/45963f45d1b1bfb37476d902f201fbdcd7597038/pineappl/src/grid.rs#L1262-L1263

Sorry to reply so late. I would agree with you, but at the moment it is asymmetric: the reason is that I didn't implement the pids detection in self.axes() yet, but we are relying on that information in self.convolute_eko nevertheless.

Most likely the easiest solution is the following: I implement as soon as possible actual pids detection, and we do as you said.
There is no reason why after querying the information needed, the eko coming back is not compliant, and if it's not compliant it would not be meaningful to convolute it with the current Grid.

[alecandido]

I tried to add a test for convolute_eko, that, as you can see, is now failing.

It would be quite useful to have such a test, because while working on convolute_eko I can check that it is at least still running simply using cargo test, instead of having to run a python program fishing an actual grid somewhere.

I'm stuck with something happening inside lagrange_subgrid, I'm reaching an unreachable!(), most likely because of the x_grid, but I would like to keep it extremely small, if possible 1 point (if not there is already the code to generate e (1, 3, 3, 3, 3) operator, commented).

Any hint? @cschwan

[cschwan]

@alecandido The problem is that you're hitting a few corner cases here:

• If the grid has only one point (ny1 = 1), then we're hitting the following problem here: https://github.com/N3PDF/pineappl/blob/c6c5a0bb6f45480b3c4213fcbd84b73b04ba228d/pineappl/src/lagrange_subgrid.rs#L392-L394 this will cause a division by zero (fine for f64), and the equation that is solved numerically in https://github.com/N3PDF/pineappl/blob/c6c5a0bb6f45480b3c4213fcbd84b73b04ba228d/pineappl/src/lagrange_subgrid.rs#L18-L32 will find no answer for a NaN. You should be able to avoid this problem by using at least two grid points.
• A further problem is that although you're filling the grid, you're setting Q^2 = 20 GeV^2, which is below the threshold of Q^2_min = 100 GeV^2. That means your grid is empty; to avoid those situations in tests, I suggest you add an assertion testing that the subgrid is not empty.
• Maybe you want to avoid interpolation completely? In that case I suggest to have a look at ImportOnlySubgridV1::new and SparseArray3::from_ndarray

[alecandido]

Thank you very much for all the info, I felt that I was doing something bad, but I wanted a simple grid and I didn't want to blow up on something complicated just to avoid some errors I was not understanding.

• Maybe you want to avoid interpolation completely? In that case I suggest to have a look at ImportOnlySubgridV1::new and SparseArray3::from_ndarray

Yep, definitely.

Let me have another go, maybe I'll be able to fix it.

[cschwan]

Thank you very much for all the info, I felt that I was doing something bad, but I wanted a simple grid and I didn't want to blow up on something complicated just to avoid some errors I was not understanding.

Don't worry, I'm happy you discovered those cases. I'll fix them at some point and opened a new Issue: #76.

[alecandido]

Added a very minimal (and working) test in be69003, but at least it guarantees that convolute_eko keeps running.

[alecandido]

At this point:

• we replaced the convolute_eko input (in rust) from a lot oaf arguments to a single struct + an array
• in python still there is a plethora of arguments for convolute_eko, but I'm not sure that we want to expose EkoInfo also there, and the actual public python API (the exposed to the user), actually takes an eko.Output object as an input, so everything is inferred (eko.Output goes into many arguments, those are unpacked to EkoInfo + array)
• we have improved test coverage

Now I'm in favor to merge.

[alecandido]

We should optimize evolution basis products when they are zero by cancellation (i.e. zero more or less at float precision)

[alecandido]

As I said, at the moment we are relying in multiple places on the strings:

• "pdg_mc_ids"
• "evol"
  but they are not properly written in any single place, so you might worry which are the allowed values.

I would add an enum somewhere with just two variants PdgMcIds and Evol or something like, and add a couple of translate functions, to go from and to strings.
In this way we could use that type in several places, having the advantage that in order to provide the type you have to use translation, and that will be a single source of truth.

[cschwan]

We should optimize evolution basis products when they are zero by cancellation (i.e. zero more or less at float precision)

I agree. Do we want to do this here, or open a new Issue? I'm leaning towards a new Issue since more optimization isn't necessarily needed for the time being.

[cschwan]

As I said, at the moment we are relying in multiple places on the strings:

* "pdg_mc_ids"

* "evol"
  but they are not properly written in any single place, so you might worry which are the allowed values.

I would add an enum somewhere with just two variants PdgMcIds and Evol or something like, and add a couple of translate functions, to go from and to strings. In this way we could use that type in several places, having the advantage that in order to provide the type you have to use translation, and that will be a single source of truth.

I agree, strings are weakly typed and we should avoid them in Rust/... as much as possible. I'm going to open a separate Issue for this.

[alecandido]

Good, I'm fine with separate issues for both, it will make it easier to track progresses. This PR is already consumed (i.e. it was dedicated to something else).

[cschwan]

We should optimize evolution basis products when they are zero by cancellation (i.e. zero more or less at float precision)

This is now part of #45.

[alecandido]

Good, then I'm going to merge this one.

[cschwan]

As I said, at the moment we are relying in multiple places on the strings:

* "pdg_mc_ids"

* "evol"
  but they are not properly written in any single place, so you might worry which are the allowed values.

I would add an enum somewhere with just two variants PdgMcIds and Evol or something like, and add a couple of translate functions, to go from and to strings. In this way we could use that type in several places, having the advantage that in order to provide the type you have to use translation, and that will be a single source of truth.

This is now #78.