search

NNPDF / pineappl

PineAPPL is not an extension of APPLgrid
https://nnpdf.github.io/pineappl/
GNU General Public License v3.0
12 stars 3 forks source link

Accommodate for two different EKOs in Evolution #289

Open Radonirinaunimi opened 1 month ago

Radonirinaunimi commented 1 month ago

Addresses https://github.com/NNPDF/pineappl/issues/288. So far, this is just a bare brute-force implementation to test if it conceptually works.

Remaining to do:

cschwan commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/NNPDF/pineappl/pull/289#issuecomment-2141576405.

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 commented 1 month ago

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: https://github.com/NNPDF/pineappl/blob/95b3bc36c477d379744b0dd1ad834d45b10e15b0/pineappl/src/grid.rs#L1400-L1406

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 commented 1 month ago

@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 commented 1 month ago

@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 :smile:.

cschwan commented 1 month ago

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 commented 1 month ago

Hi all, since now we made quite a few progress in https://github.com/NNPDF/pineko/pull/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 commented 1 month ago

@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 https://github.com/NNPDF/pineko/pull/181. In this sense, at least, I think this can be reviewed. What do you think?

Radonirinaunimi commented 1 month ago

So the status is that with the modifications in https://github.com/NNPDF/pineko/pull/181, this works fine. Below, for instance, is a comparison between the grids (with [p]PDF evolution) and the FK tables (with EKOs) for the observable $\Delta \sigma (p{\mathrm{pol}} \otimes p{\mathrm{unpol}})$.

*********************************
Th: 823  STAR_WMWP_510GEV_WM-AL-POL
*********************************
        FKTable          Grid  abs_diff  rel_diff
0    803.274230    803.188890  0.085339  0.010625
1   5864.619611   5864.785764 -0.166153 -0.002833
2  14206.376784  14206.816815 -0.440031 -0.003097
3  20814.990536  20815.514610 -0.524074 -0.002518
4  19516.276622  19517.356552 -1.079930 -0.005533
5   7529.690834   7531.065920 -1.375087 -0.018259

Will now address all the comments above and add tests.

cschwan commented 1 month ago

Will now address all the comments above and add tests.

You can add a test similar to other tests, for instance like:

https://github.com/NNPDF/pineappl/blob/cca611189eb420bb0bfe26b96e3b6bbdec0c5a11/pineappl_cli/tests/evolve.rs#L429-L446

Simply create another sensibly-named test function, add a new static string with the test output and set the argument of stderr to "".

For that to work in the CI,

Radonirinaunimi commented 4 weeks ago

Thanks @cschwan! I will follow this.

felixhekhorn commented 4 weeks ago

Will now address all the comments above and add tests.

You can add a test similar to other tests, for instance like:

https://github.com/NNPDF/pineappl/blob/cca611189eb420bb0bfe26b96e3b6bbdec0c5a11/pineappl_cli/tests/evolve.rs#L429-L446

Simply create another sensibly-named test function, add a new static string with the test output and set the argument of stderr to "".

For that to work in the CI,

* upload both operators and the grid to https://data.nnpdf.science/pineappl/test-data/,

* add another `curl` invocation in the Rust action here: https://github.com/NNPDF/pineappl/blob/cca611189eb420bb0bfe26b96e3b6bbdec0c5a11/.github/workflows/rust.yml#L32
   and also in the bash script: https://github.com/NNPDF/pineappl/blob/cca611189eb420bb0bfe26b96e3b6bbdec0c5a11/maintainer/generate-coverage.sh#L7
   with `wget`, and finally

* increment the test-data version number here: https://github.com/NNPDF/pineappl/blob/cca611189eb420bb0bfe26b96e3b6bbdec0c5a11/.github/workflows/rust.yml#L26

@cschwan I wonder if you should add these instructions to https://github.com/NNPDF/pineappl/blob/master/maintainer/README.md ?

cschwan commented 4 weeks ago

Yes, that's a good idea; I'll add them to CONTRIBUTING.md, since every contributor can potentially add these kind of tests.

Radonirinaunimi commented 3 weeks ago

The test is failing because the container registry needs to be updated. I am not sure who has the permission to do so? Maybe we should also add something in this regard in the CONTRIBUTING.md?

cschwan commented 3 weeks ago

I added the instructions with commit 644677b858615f22afb1fbda2dc00518b818e6c3. I suggest we first try fix the problems without updating the container, because this usually isn't as straightforward as it should be. @Radonirinaunimi please fix the failing test that isn't affected by the missing PDFs. I will try to fix the other problems.

cschwan commented 3 weeks ago

@Radonirinaunimi in commit 5630f708fc96138afee785eba3f5a57caa5ac292 you've changed the implementation of the convolution function of the CLI, which heavily overlaps with what I'm doing with @t7phy in #293. For the being this is OK, because the test is more important, but we should make sure that we first merge #293 into master, master into this branch and then this branch into master. I'll add a TODO item for this above.

Radonirinaunimi commented 3 weeks ago

I added the instructions with commit 644677b.

Perfect, thanks!

I suggest first try fix the problems without updating the container, because this usually isn't as straightforward as it should be. @Radonirinaunimi please fix the test that's not related to fetching the PDFs, I will try to fix the other problems.

Ok, I think I will just move the line here: https://github.com/NNPDF/pineappl/blob/5630f708fc96138afee785eba3f5a57caa5ac292/maintainer/pineappl-ci/script.sh#L55-L56 to rust.yml for the time being then. For the test to pass, we really need a PDF set evolved with the same EKO.

@Radonirinaunimi in commit https://github.com/NNPDF/pineappl/commit/5630f708fc96138afee785eba3f5a57caa5ac292 you've changed the implementation of the convolution function of the CLI, which heavily overlaps with what I'm doing with @t7phy in https://github.com/NNPDF/pineappl/pull/293. For the being I'm OK with leaving it as is, but we should make sure that we first merge https://github.com/NNPDF/pineappl/pull/293 into master, master into this branch and then this branch into master. I'll add a TODO item for this above.

Yes, I was indeed aware of this and I extended the CLI just for the sake of running tests but having in mind that this will be overwritten. We simply need to revert some of the changes in 5630f70. So the plans and todos sound good. Just for info, about when do you expect https://github.com/NNPDF/pineappl/pull/293 to get merged?

cschwan commented 3 weeks ago

I added the instructions with commit 644677b.

Perfect, thanks!

I suggest first try fix the problems without updating the container, because this usually isn't as straightforward as it should be. @Radonirinaunimi please fix the test that's not related to fetching the PDFs, I will try to fix the other problems.

Ok, I think I will just move the line here:

https://github.com/NNPDF/pineappl/blob/5630f708fc96138afee785eba3f5a57caa5ac292/maintainer/pineappl-ci/script.sh#L55-L56

to rust.yml for the time being then. For the test to pass, we really need a PDF set evolved with the same EKO.

Right, that's even simpler than what I thought of; please go ahead with this.

@Radonirinaunimi in commit 5630f70 you've changed the implementation of the convolution function of the CLI, which heavily overlaps with what I'm doing with @t7phy in #293. For the being I'm OK with leaving it as is, but we should make sure that we first merge #293 into master, master into this branch and then this branch into master. I'll add a TODO item for this above.

Yes, I was indeed aware of this and I extended the CLI just for the sake of running tests but having in mind that this will be overwritten. We simply need to revert some of the changes in 5630f70. So the plans and todos sound good. Just for info, about when do you expect #293 to get merged?

Pretty soon I hope.

cschwan commented 1 day ago

@Radonirinaunimi the newest container now has the polarized PDF included, if you like you can test that by removing the PDF set from the action.

Radonirinaunimi commented 23 hours ago

@Radonirinaunimi the newest container now has the polarized PDF included, if you like you can test that by removing the PDF set from the action.

Thanks a lot @cschwan! I've removed the download of the pPDF set from rust.yml.