Closed alecandido closed 2 years ago
Merging #75 (c422fb6) into master (02c4f45) will increase coverage by
0.68%
. The diff coverage is60.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%) |
:arrow_up: |
pineappl/src/lumi.rs | 16.66% <0.00%> (-83.34%) |
:arrow_down: |
pineappl/src/pids.rs | 0.00% <0.00%> (ø) |
|
pineappl/src/empty_subgrid.rs | 94.11% <0.00%> (+11.76%) |
:arrow_up: |
pineappl/src/fk_table.rs | 38.96% <0.00%> (+38.96%) |
:arrow_up: |
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.
@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 + commit c42860c: Please don't do that, instead add metadata to the
Grid
thatconvolute_eko
creates. The argument list ofconvolute_eko
is already far too long.
We thought about it, but we do like the alternative even less:
set_key_value
should not be allowed)@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).
@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 ofconvolute_eko
. The advantage of that over aHashMap
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...).
Let's recap what we need here to merge, not leave things open forever:
convolute_eko
to a suitable struct (to be introduced)Is there anything else?
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
@AleCandido thanks, the EkoInfo
struct was really wrongly named, but now it correctly reflects the fact that it's information from EKO.
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.
@AleCandido
grid_axes
member in EkoInfo
? I think we can simply use let grid_axes = self.axes()?;
, which we already use in any case:
- do you really need the
grid_axes
member inEkoInfo
? I think we can simply uselet grid_axes = self.axes()?;
, which we already use in any case:
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
.
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
@AleCandido The problem is that you're hitting a few corner cases here:
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.ImportOnlySubgridV1::new
and SparseArray3::from_ndarray
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
andSparseArray3::from_ndarray
Yep, definitely.
Let me have another go, maybe I'll be able to fix it.
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: https://github.com/N3PDF/pineappl/issues/76.
Added a very minimal (and working) test in be69003, but at least it guarantees that convolute_eko
keeps running.
At this point:
convolute_eko
input (in rust) from a lot oaf arguments to a single struct + an arrayconvolute_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)Now I'm in favor to merge.
We should optimize evolution basis products when they are zero by cancellation (i.e. zero more or less at float precision)
As I said, at the moment we are relying in multiple places on the strings:
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.
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.
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 variantsPdgMcIds
andEvol
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.
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).
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 https://github.com/N3PDF/pineappl/issues/45.
Good, then I'm going to merge this one.
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 variantsPdgMcIds
andEvol
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 https://github.com/N3PDF/pineappl/issues/78.
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)