alecandido commented 3 years ago

Hi @cschwan, I was reading about PyO3, and after a while I started thinking that it may be not so hard to provide bindings for pineappl using it.

I don't know how much you care, but I decided to make a try, just in my spare time. The goal it's to port a minimal subset of things, just a proof of concept, to expand after to the full one.

It should not take so much, but of course it's not my main business, so it will take its time. Do you mind if I try?

I also thought it was a good way to familiarize a little more with pineappl, since soon it will be useful (both because of yadism, but also because I am going to work on pineko too, and I know you are the expert on pineappl, but it won't hurt to know something more).

If you agree maybe you can suggest me which one could be "a minimal set".

cschwan commented 3 years ago

Hi @AleCandido , I don't mind at all, and in fact I think that would be very useful. Go ahead, and let me know if you need to know anything.

For a minimal set of structs and methods, just have a look at #50 , in particular the file https://github.com/N3PDF/pineappl/pull/50/files#diff-4a3c785edeb3672939e989f1aedce3a888747e39fabed535fba4a7909f13f7b2. This file more or less contains everything important from the user side.

Don't forget to change the author of pineappl_py to yourself.

alecandido commented 3 years ago

First, small, trade off due to python + rust interaction:

maturin, the zero-configuration tool for building and deploying pyo3 packages, it's enforcing a specific layout, and it is perfectly fine
unfortunately, the name of the PyPI package will be the same of the rust package:
- in both the case of a pure rust package and a mixed python/rust one you are free to choose (the same) unrestricted name for the import package (i.e. the name you use when you do in python import my_package)
- but there is another name involved: the one you use to do pip install my-package, and it's the distribution package

And now the problem: while the current python package it's named pineappl (and it makes sense), now we can't choose that name any longer, using maturin, because, since it's extracted from Cargo.toml package.name, picking pineappl would conflict with the other one (the original) in the workspace.

Maybe we can overcome this little pain by using setuptools_rust, but it's far more verbose and requires many more files (i.e. at least a couple), rather than the simple standard/enforced layout of maturin.

Is it worth? (we can call pineappl_py the distr package and still doimport pineappl`, i.e. what I'm currently doing)

cschwan commented 3 years ago

It's probably best to rename it, since keeping the name of the other python package suggest they're compatible, which they probably won't.

alecandido commented 3 years ago

You're right, we can think a little on the name (pineappl_py, pineappl-py, pyneappl, ...), but it's just the installation one for distribution package, so you will simply find pineappl_py when you search on PyPI, slightly uglier but it has the further advantage of being explicit: it's not shipping the rust lib only (possible for PyPI but useless) but also the proper bindings, even if it's of course expected from a PyPI package...

And it will keep the complexity low, without any extra than bindings themselves.

cschwan commented 3 years ago

Do we actually need a separate crate for the python bindings? It might be beneficial to put everything into the main crate, preferably only when the python feature is activated. In this way there's less code duplication and adding a method is easy (?!).

alecandido commented 3 years ago

You're right, doing it in the main crate would be easier, but when I decided to split I made two considerations:

maybe python it's not so special, so why is the C one split and python should fit in the main one? And in this way the python would be rather more coupled, because the development of python wrappers it's somehow interleaved in the library itself
I didn't want to mess up with the library myself, because in this way I was completely sure that I wasn't ruining anything at all

On the other hand there are some pros:

there are examples of other projects doing the same
maturin itself it's supporting this layout
I probably need some python code for making proper classes (because of limitations of pyo3 and the struct-class mismatch) and for this I had to make a pineappl package (it would be pineappl/pineappl/pineappl inside the main crate)

Nevertheless being outside the main crate I need to wrap all the structs and functions to annotate them, and it's a pain in the neck, so it would be much better to use the main crate just for this reason.

What do you think?

cschwan commented 3 years ago

If it isn't master you can do whatever you like. I'm more concerned that whenever I add new Rust code in the future, I'll have to update lots of code in other places. This already happens for the C API, which definitely doesn't support all methods the Rust interface has. I was already thinking about merging the pineappl_capi into pineappl. But for the time being we should focus on adding the functionality.

alecandido commented 3 years ago

Fine. Let's say that since I started this way I'll try to complete just the proof of concept, with the goal of running import-yadism-yaml.py with the new wrapper.

As soon as I get there we can merge in dis and than decide the best layout. Just not to get lost in changes.

cschwan commented 3 years ago

Agreed!

alecandido commented 3 years ago

Help/advices appreciated:

I'm writing a lot of repeated code: how should I refactor it? is there room for a suitable macro?

Moreover there are still some errors to fix, because of using custom structs in the signature of python targeted functions (but I'll try to take care of this).

alecandido commented 3 years ago

Some errors fixed according to this issue, last one missing (missing Clone trait, I'll found a way), and refactoring. The moment I'm able to compile I'll write a few unit tests (for python, I'd say that in rust there is nothing to test) and I'll transpile the script.

alecandido commented 3 years ago

Everything available and exported, the module compiles: now I'm missing just the python module.

A couple of todos:

lumi_entry! it's a macro, but of course there is nothing like a macro in python (no preprocessing, no compilation -> everything at runtime) so I'll supply a suitable constructor for the object, and that's it
SubgridParams are completely hidden: fields are private and accessed by getters and setters, so I did the full copy explicit to implement it, since it seems like PyO3 requires Clone trait for the python functions argument

https://github.com/N3PDF/pineappl/blob/9d671cfad5d03754bd7e189f897f2805131f3d4b/pineappl_py/src/subgrid.rs#L17-L31

For the time being I would have liked to make it works without touching pineappl at all, but I'd suggest to derive Clone and also Copy for SubgridParams: it's just a collection of scalars, so there is nothing preventing to do the copy on the stack.

alecandido commented 3 years ago

Hi @cschwan the last commit 45a8f8b made import_yadism_yaml.py able to dump a pineappl grid for the first time, but still I have to solve few things (like properly wrapping some methods for easiness of use in python).

I still have a couple of issues to be solved:

in order to expose a python object, let's say PyLagrangeSubgridV2, it needs to be clonable, so I deserved to implement the Clone trait on it; for the time being I simply derived it, but in order to derive all its attributes had to be clonable as well, so I derived the trait also for pineappl struct as well; https://github.com/N3PDF/pineappl/blob/45a8f8bf9d655baf54714e78d1ddc5f821f90414/pineappl/src/lagrange_subgrid.rs#L331-L332 do you have any suggestion on how to make a clonable struct wrapping a non-clonable one?
I had to dump on a file, so a file descriptor was required; actually the best thing would be to be able to open the file descriptor directly in python, and pass it down, but I currently found no way, so I just passed the string with the path and did it in rust; I'm open for advices also here

If you wish try to run on the old data you already used for the previous benchmark to run both the scripts (import_yadism_yaml.rs and import_yadism_yaml.py) and make the grids comparison :)

cschwan commented 3 years ago

Hi @AleCandido , concerning your first point I'd suggest you simply add Clone where it's needed. I don't know how to treat non-clonable structs - can't we make them Clone as well? The only other way would be to use some smart pointer, I suppose.

As for the python file descriptors I've found this: https://github.com/PyO3/pyo3/issues/933; but the upshot seems that it's not yet properly supported yet.

alecandido commented 3 years ago

Fine, thank you very much.

The solution to the first point it's the currently implemented one. You are right, probably the only other way it's to go through smart pointers, but it would be rather complicated: we need someone to own and the possible copies to share ownership without moving, so something like Rc with RefCell, but it's really going to be too complicated w.r.t. just adding Clone to the original structs.

For the second one I'm sorry, but then maybe the current solution of passing down the path and delegate everything to rust it's the only one.

felixhekhorn commented 3 years ago

Ciao @cschwan , when trying to run @AleCandido s work I obtain something strange:

(env) felix@FHe19b:~/Physik/N3PDF/PineAPPL/pineappl/pineappl_py/examples$ pineappl convolute test.pineappl.grid CT14llo_NF6
LHAPDF 6.2.3 loading /home/felix/local/share/LHAPDF/CT14llo_NF6/CT14llo_NF6_0000.dat
CT14llo_NF6 PDF set, member #0, version 1; LHAPDF ID = 13208
bin                  x1                                       x2                          diff        integ     neg unc pos unc
---+------------------+------------------+---------------------+---------------------+------------+------------+-------+-------
  0                 90                 90  0.000317313166918093  0.000317313166918093 1.3541281e-5 1.3541281e-5 -32.91%  35.08%
  1                 90                 90 0.0010068764589958945 0.0010068764589958945 5.3658298e-5 5.3658298e-5 -28.13%  28.03%
  2                 90                 90 0.0031949515789926235 0.0031949515789926235 2.0887563e-4 2.0887563e-4 -22.62%  20.91%
  3                 90                 90  0.010138002036801111  0.010138002036801111 8.2579327e-4 8.2579327e-4 -15.86%  13.45%
  4                 90                 90   0.03216921532519438   0.03216921532519438 3.5360319e-3 3.5360319e-3  -7.94%   5.95%
  5                 90                 90   0.10207715592107469   0.10207715592107469 1.7796234e-2 1.7796234e-2  -0.68%   0.00%
  6                 90                 90   0.30454545454545456   0.30454545454545456 1.1354667e-1 1.1354667e-1  -8.83%  10.70%
  7                 90                 90    0.5363636363636364    0.5363636363636364 2.7885576e-1 2.7885576e-1 -16.31%  22.90%
  8                 90                 90    0.7681818181818182    0.7681818181818182 3.5127562e-1 3.5127562e-1 -24.46%  38.99%
  9                 90                 90                     1                     1  0.0000000e0  0.0000000e0    NaN%    NaN%
 10                  4                  4                 0.001                 0.001 2.0633767e-5 2.0633767e-5 -34.79%  34.15%
 11 5.3489354902404145 5.3489354902404145                 0.001                 0.001 2.5276456e-5 2.5276456e-5 -36.04%  49.37%
 12  7.152777719688369  7.152777719688369                 0.001                 0.001 2.7715123e-5 2.7715123e-5 -36.09%  46.31%
 13  9.564936649660504  9.564936649660504                 0.001                 0.001 3.0269254e-5 3.0269254e-5 -35.90%  43.29%
 14  12.79055727681758  12.79055727681758                 0.001                 0.001 3.2913465e-5 3.2913465e-5 -35.16%  40.42%
 15 17.103966439480587 17.103966439480587                 0.001                 0.001 3.5627476e-5 3.5627476e-5 -34.05%  41.02%
 16 22.872003278004676 22.872003278004676                 0.001                 0.001 3.8400967e-5 3.8400967e-5 -32.75%  39.07%
 17  30.58521751665358  30.58521751665358                 0.001                 0.001 4.1512654e-5 4.1512654e-5 -31.85%  36.25%
 18  40.89958886288778  40.89958886288778                 0.001                 0.001 4.4653744e-5 4.4653744e-5 -30.87%  33.71%
 19 54.692315601235514 54.692315601235514                 0.001                 0.001 4.7806214e-5 4.7806214e-5 -29.86%  31.45%
 20  73.13641699071955  73.13641699071955                 0.001                 0.001 5.0967572e-5 5.0967572e-5 -28.86%  29.41%
 21  97.80049411767047  97.80049411767047                 0.001                 0.001 5.4128791e-5 5.4128791e-5 -27.78%  27.57%
 22 130.78213348726425 130.78213348726425                 0.001                 0.001 5.7283965e-5 5.7283965e-5 -26.27%  25.90%
 23  174.8862988248468  174.8862988248468                 0.001                 0.001 6.0429177e-5 6.0429177e-5 -24.91%  24.38%
 24  233.8638826352534  233.8638826352534                 0.001                 0.001 6.3558142e-5 6.3558142e-5 -23.64%  23.00%
 25  312.7307054282815  312.7307054282815                 0.001                 0.001 6.6667632e-5 6.6667632e-5 -22.46%  21.74%
 26 418.19409228831387 418.19409228831387                 0.001                 0.001 6.9756415e-5 6.9756415e-5 -21.36%  20.58%
 27  559.2233055124594  559.2233055124594                 0.001                 0.001 7.2818741e-5 7.2818741e-5 -20.34%  19.52%
 28  747.8123464562881  747.8123464562881                 0.001                 0.001 7.5854495e-5 7.5854495e-5 -19.39%  18.54%
 29               1000               1000                 0.001                 0.001 7.8861691e-5 7.8861691e-5 -18.50%  17.64%
Thanks for using LHAPDF 6.2.3. Please make sure to cite the paper:
  Eur.Phys.J. C75 (2015) 3, 132  (http://arxiv.org/abs/1412.7420)

everything seems to be double - and where is the actual result of the convolution?

(I'm using the pineappl version from this branch)

alecandido commented 3 years ago

I now installed pineappl_cli the same way of @felixhekhorn, i.e. using

cargo install --path <path-to-local-pineappl_cli>

and it's working the same way.

Instead using:

cargo install pineappl_cli

I obtained:

Error: invalid value: integer `3`, expected variant index 0 <= i < 2

cschwan commented 3 years ago

@felixhekhorn : I can reproduce your numbers. The actual result of the convolution is shown as integ (shorthand for integrated result) and also in diff (for differential result, which is integ divided by the bin width or, in this case, divided by one if the witdth is zero).

The numbers from the Rust program are quite different, so there must be something wrong with the python program.

@AleCandido: If you use cargo install pineappl_cli then you install the latest version published to crates.io, but that version doesn't understand the new grid formats yet.

cschwan commented 3 years ago

Commit a8f8798fd9285fa4d8ef7f23f5d6c4b3a77024de fixes the problem.

alecandido commented 3 years ago

The numbers from the Rust program are quite different, so there must be something wrong with the python program.

Actually @cschwan these are the numbers I got:

❯ pineappl convolute rust.pineappl.grid CT14llo_NF6                                                                                  │❯ pineappl convolute python.pineappl.grid CT14llo_NF6
LHAPDF 6.2.3 loading /home/alessandro/.local/share/LHAPDF/CT14llo_NF6/CT14llo_NF6_0000.dat                                           │LHAPDF 6.2.3 loading /home/alessandro/.local/share/LHAPDF/CT14llo_NF6/CT14llo_NF6_0000.dat
CT14llo_NF6 PDF set, member #0, version 1; LHAPDF ID = 13208                                                                         │CT14llo_NF6 PDF set, member #0, version 1; LHAPDF ID = 13208
bin                  x1                                       x2                          diff        integ     neg unc pos unc      │bin                  x1                                       x2                          diff        integ     neg unc pos unc
---+------------------+------------------+---------------------+---------------------+------------+------------+-------+-------      │---+------------------+------------------+---------------------+---------------------+------------+------------+-------+-------
  0                 90                 90  0.000317313166918093  0.000317313166918093  2.3452168e0  2.3452168e0 -32.77%  34.84%      │  0                 90                 90  0.000317313166918093  0.000317313166918093  2.3452168e0  2.3452168e0 -32.77%  34.84%
  1                 90                 90 0.0010068764589958945 0.0010068764589958945  1.6402034e0  1.6402034e0 -27.98%  27.80%      │  1                 90                 90 0.0010068764589958945 0.0010068764589958945  1.6402034e0  1.6402034e0 -27.98%  27.80%
  2                 90                 90 0.0031949515789926235 0.0031949515789926235  1.1307150e0  1.1307150e0 -22.51%  20.77%      │  2                 90                 90 0.0031949515789926235 0.0031949515789926235  1.1307150e0  1.1307150e0 -22.51%  20.77%
  3                 90                 90  0.010138002036801111  0.010138002036801111 7.8100374e-1 7.8100374e-1 -15.82%  13.40%      │  3                 90                 90  0.010138002036801111  0.010138002036801111 7.8100374e-1 7.8100374e-1 -15.82%  13.40%
  4                 90                 90   0.03216921532519438   0.03216921532519438 5.5404832e-1 5.5404832e-1  -7.89%   5.91%      │  4                 90                 90   0.03216921532519438   0.03216921532519438 5.5404832e-1 5.5404832e-1  -7.89%   5.91%
  5                 90                 90   0.10207715592107469   0.10207715592107469 3.9543367e-1 3.9543367e-1  -0.65%   0.00%      │  5                 90                 90   0.10207715592107469   0.10207715592107469 3.9543367e-1 3.9543367e-1  -0.65%   0.00%
  6                 90                 90   0.30454545454545456   0.30454545454545456 2.3020618e-1 2.3020618e-1  -8.83%  10.70%      │  6                 90                 90   0.30454545454545456   0.30454545454545456 2.3020618e-1 2.3020618e-1  -8.83%  10.70%
  7                 90                 90    0.5363636363636364    0.5363636363636364 7.3231793e-2 7.3231793e-2 -16.31%  22.90%      │  7                 90                 90    0.5363636363636364    0.5363636363636364 7.3231793e-2 7.3231793e-2 -16.31%  22.90%
  8                 90                 90    0.7681818181818182    0.7681818181818182 7.1674283e-3 7.1674283e-3 -24.48%  39.03%      │  8                 90                 90    0.7681818181818182    0.7681818181818182 7.1674283e-3 7.1674283e-3 -24.48%  39.03%
  9                 90                 90                     1                     1  0.0000000e0  0.0000000e0    NaN%    NaN%      │  9                 90                 90                     1                     1  0.0000000e0  0.0000000e0    NaN%    NaN%
 10                  4                  4                 0.001                 0.001 6.4036816e-1 6.4036816e-1 -34.80%  33.96%      │ 10                  4                  4                 0.001                 0.001 6.4036816e-1 6.4036816e-1 -34.80%  33.96%
 11 5.3489354902404145 5.3489354902404145                 0.001                 0.001 7.8419947e-1 7.8419947e-1 -36.03%  49.07%      │ 11 5.3489354902404145 5.3489354902404145                 0.001                 0.001 7.8419947e-1 7.8419947e-1 -36.03%  49.07%
 12  7.152777719688369  7.152777719688369                 0.001                 0.001 8.5954382e-1 8.5954382e-1 -36.04%  46.01%      │ 12  7.152777719688369  7.152777719688369                 0.001                 0.001 8.5954382e-1 8.5954382e-1 -36.04%  46.01%
 13  9.564936649660504  9.564936649660504                 0.001                 0.001 9.3837339e-1 9.3837339e-1 -35.81%  43.00%      │ 13  9.564936649660504  9.564936649660504                 0.001                 0.001 9.3837339e-1 9.3837339e-1 -35.81%  43.00%
 14  12.79055727681758  12.79055727681758                 0.001                 0.001  1.0199076e0  1.0199076e0 -35.05%  40.13%      │ 14  12.79055727681758  12.79055727681758                 0.001                 0.001  1.0199076e0  1.0199076e0 -35.05%  40.13%
 15 17.103966439480587 17.103966439480587                 0.001                 0.001  1.1035237e0  1.1035237e0 -33.93%  40.72%      │ 15 17.103966439480587 17.103966439480587                 0.001                 0.001  1.1035237e0  1.1035237e0 -33.93%  40.72%
 16 22.872003278004676 22.872003278004676                 0.001                 0.001  1.1889070e0  1.1889070e0 -32.61%  38.77%      │ 16 22.872003278004676 22.872003278004676                 0.001                 0.001  1.1889070e0  1.1889070e0 -32.61%  38.77%
 17  30.58521751665358  30.58521751665358                 0.001                 0.001  1.2846396e0  1.2846396e0 -31.70%  35.97%      │ 17  30.58521751665358  30.58521751665358                 0.001                 0.001  1.2846396e0  1.2846396e0 -31.70%  35.97%
 18  40.89958886288778  40.89958886288778                 0.001                 0.001  1.3812124e0  1.3812124e0 -30.72%  33.45%      │ 18  40.89958886288778  40.89958886288778                 0.001                 0.001  1.3812124e0  1.3812124e0 -30.72%  33.45%
 19 54.692315601235514 54.692315601235514                 0.001                 0.001  1.4780730e0  1.4780730e0 -29.71%  31.20%      │ 19 54.692315601235514 54.692315601235514                 0.001                 0.001  1.4780730e0  1.4780730e0 -29.71%  31.20%
 20  73.13641699071955  73.13641699071955                 0.001                 0.001  1.5751482e0  1.5751482e0 -28.71%  29.17%      │ 20  73.13641699071955  73.13641699071955                 0.001                 0.001  1.5751482e0  1.5751482e0 -28.71%  29.17%
 21  97.80049411767047  97.80049411767047                 0.001                 0.001  1.6721643e0  1.6721643e0 -27.63%  27.34%      │ 21  97.80049411767047  97.80049411767047                 0.001                 0.001  1.6721643e0  1.6721643e0 -27.63%  27.34%
 22 130.78213348726425 130.78213348726425                 0.001                 0.001  1.7689426e0  1.7689426e0 -26.12%  25.68%      │ 22 130.78213348726425 130.78213348726425                 0.001                 0.001  1.7689426e0  1.7689426e0 -26.12%  25.68%
 23  174.8862988248468  174.8862988248468                 0.001                 0.001  1.8653650e0  1.8653650e0 -24.76%  24.18%      │ 23  174.8862988248468  174.8862988248468                 0.001                 0.001  1.8653650e0  1.8653650e0 -24.76%  24.18%
 24  233.8638826352534  233.8638826352534                 0.001                 0.001  1.9612405e0  1.9612405e0 -23.50%  22.80%      │ 24  233.8638826352534  233.8638826352534                 0.001                 0.001  1.9612405e0  1.9612405e0 -23.50%  22.80%
 25  312.7307054282815  312.7307054282815                 0.001                 0.001  2.0564734e0  2.0564734e0 -22.32%  21.55%      │ 25  312.7307054282815  312.7307054282815                 0.001                 0.001  2.0564734e0  2.0564734e0 -22.32%  21.55%
 26 418.19409228831387 418.19409228831387                 0.001                 0.001  2.1510280e0  2.1510280e0 -21.23%  20.40%      │ 26 418.19409228831387 418.19409228831387                 0.001                 0.001  2.1510280e0  2.1510280e0 -21.23%  20.40%
 27  559.2233055124594  559.2233055124594                 0.001                 0.001  2.2447310e0  2.2447310e0 -20.21%  19.35%      │ 27  559.2233055124594  559.2233055124594                 0.001                 0.001  2.2447310e0  2.2447310e0 -20.21%  19.35%
 28  747.8123464562881  747.8123464562881                 0.001                 0.001  2.3375806e0  2.3375806e0 -19.26%  18.38%      │ 28  747.8123464562881  747.8123464562881                 0.001                 0.001  2.3375806e0  2.3375806e0 -19.26%  18.38%
 29               1000               1000                 0.001                 0.001  2.4295179e0  2.4295179e0 -18.37%  17.48%      │ 29               1000               1000                 0.001                 0.001  2.4295179e0  2.4295179e0 -18.37%  17.48%
Thanks for using LHAPDF 6.2.3. Please make sure to cite the paper:                                                                   │Thanks for using LHAPDF 6.2.3. Please make sure to cite the paper:
  Eur.Phys.J. C75 (2015) 3, 132  (http://arxiv.org/abs/1412.7420)                                                                    │  Eur.Phys.J. C75 (2015) 3, 132  (http://arxiv.org/abs/1412.7420)

where rust.pineappl.grid has been produced by the following command:

cargo run --example import-yadism-yaml pineappl_data/pineappl_sample.yaml rust.pineappl.grid

and python.pineappl.grid by this other one:

python3 import-yadism-yaml.py ../../pineappl_data/pineappl_sample.yaml python.pineappl.grid

So they look quite the same. Of course using in both cases the same input data, taken from the other PR #50, and the pineappl version of this branch (both for rust and python versions, and for CLI too).

Is there any other reason why you don't obtain the same numbers?

cschwan commented 3 years ago

@AleCandido I could reproduce your results. Before the commit, however, the numbers from the Python and the Rust program don't agree with each other.

alecandido commented 3 years ago

@cschwan sorry, I misunderstood, I read as the commit was fixing the CLI. Actually I read the content of the commit this afternoon, but I didn't noticed the comment. Then I read it and I didn't connect the connect and the timeline. I'm stupid :zany_face:

cschwan commented 3 years ago

@AleCandido Don't worry - I'm just glad we're on the same page and that the two programs do the same!

alecandido commented 3 years ago

@cschwan: then, for what concern yadism, we are done, even if there are still few things left (if ever):

extend the bindings to the rest of pineappl
decide some details relative to the package organization (package name, deployment, docs)
we can add some more tests to have an automatic suite to detect problems, but it's really second order
- there already some, and of course I'm not testing pineappl, that is already tested in the proper place, but the bindings themselves
add an automatic workflow for the former point (this is even less relevant, but I can do it in no time)

If according to you no one of this thing it's so relevant, and you don't have anything to add to the list, we can even merge. Let me know.

cschwan commented 3 years ago

@AleCandido : I agree. Concerning tests, let's just use what we did above.

Before merging I would also like to see the entire result polished and the amount of boilerplate code minimised. For instance, right now the approach has Rust structs, which are wrapped in other Rust structs, which in turn are wrapped in Python. Since our aim is to simplify maintenance, relative to the existing Python version, we have to simplify a bit.

alecandido commented 3 years ago

@cschwan fine, but:

the python things are very thin, essentially they're inheriting from a single class that through the usual python magic it's passing everything automatically down, so the amount of code it's really minimal and quite needed to keep it working more like python (it's also quite common to have a final python wrapper, see numpy, polars, etc.), even if it's certainly possible to use directly the raw generated module
in order to get rid of the rust struct we need to apply the bindings directly on pineappl, prefectly doable, but this means they will be interleaved in the regular code: it's a design choice and it's completely up to you

felixhekhorn commented 3 years ago

If I may add my personal point of view to the discussion: for the most part I agree with @AleCandido that this new code is a good pythonic approach - and as he said this is a design choice; but Python developers tend to use Python design rationals

Before merging I would also like to see the entire result polished and the amount of boilerplate code minimised.

I can not truely see any unnecessary or unwanted code (apart maybe from some comments, but they don't hurt)

For instance, right now the approach has Rust structs, which are wrapped in other Rust structs, which in turn are wrapped in Python. Since our aim is to simplify maintenance, relative to the existing Python version, we have to simplify a bit.

This is one specific design choice, and I think it is a good one: to couple the two worlds only loosely such they can adopt onto each other and to have a modular code structure

something that can (and should) definitely improved in the code is the documentation

alecandido commented 3 years ago

I actually agree with @felixhekhorn, but I see also the point of @cschwan in not duplicate it too much, it's quite nested at the moment. I'm open to apply the bindings to pineappl directly, they will be some attribute on structs and impl blocks:

current:


#[pyclass]
pub struct PyBinRemapper {
pub bin_remapper: BinRemapper,
}

[pymethods]

impl PyBinRemapper {

[new]

pub fn new(
//...

}

- proposed:

```rust
#[pyclass]
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct BinRemapper {
    //...
}

#[pymethods]
impl BinRemapper {
    #[new]
    pub fn new(
    //...
}

I don't know if the thing will grow even more, since you were planning to apply also C bindings directly. At the end of the day the tradeoff it's nested (but modularized) vs long (but atomic).

cschwan commented 3 years ago

Where does the #[new] macro come from?

alecandido commented 3 years ago

It's still a pyo3 element: it's tagging the method as the constructor for python.

In rust there is no default name for the constructor, since there are no classes, and the only default way to produce a struct it's using braces and specify all its attributes (if allowed). Instead in python everything it's an object, so you deserve a method to be called on creation. If #[new] it's not specified I don't know how python should be supposed to get the object.

cschwan commented 3 years ago

Thanks, I see. I didn't find it in the reference, but it's in the user guide.

cschwan commented 3 years ago

@cschwan fine, but:

* the python things are very thin, essentially they're inheriting from a single class that through the usual python magic it's passing everything automatically down, so the amount of code it's really minimal and quite needed to keep it working more like python (it's also quite common to have a final python wrapper, see numpy, polars, etc.), even if it's certainly possible to use directly the raw generated module

Can you give me some pointers why this is done? I reckon there's a good reason for it (but I fail to see one).

felixhekhorn commented 3 years ago

Can you give me some pointers why this is done? I reckon there's a good reason for it (but I fail to see one).

if you mean wrapping in an additional layer: e..g modularity: if the actual (C-/Rust-) implementation changes (due to bugs, new features, reworks) this will not effect the Python package

cschwan commented 3 years ago

Can you give me some pointers why this is done? I reckon there's a good reason for it (but I fail to see one).

if you mean wrapping in an additional layer: e..g modularity: if the actual (C-/Rust-) implementation changes (due to bugs, new features, reworks) this will not effect the Python package

I see! Although if the Rust interface changes, I think we also want the Python interface to reflect that change. And changes should only happen in major versions, of course.

alecandido commented 3 years ago

There are further reasons, related to package structure:

given that it is in python we may decide or not to expose something top-level and something not, see: https://github.com/N3PDF/pineappl/blob/a8f8798fd9285fa4d8ef7f23f5d6c4b3a77024de/pineappl_py/pineappl/__init__.py#L1-L5 python by default scope the objects in package.subpackage.[...].module scope, but you can explicitly import at higher level
we can have bindings docs from docstrings: it's true that is another thing to be maintained, but we can just describe whatever is working differently, and simply pointing to the crate docs for everything else
since rust it's compiled it may be not available on some platforms: at the end of the day you're not pointing to a pineappl installation on your system, obtained with cargo, but to the package downloaded from PyPI, if anything goes wrong python it's still around, so it can be more helpful https://github.com/N3PDF/pineappl/blob/a8f8798fd9285fa4d8ef7f23f5d6c4b3a77024de/pineappl_py/pineappl/bin.py#L1-L6
since macros are not available, sometimes you may have useful syntax implemented for rust that you can't port in any way to python, so you may wonder to implement directly in python
- or sometimes there are things that are available to python that are not available in rust, like default parameters and so on, and you may like to use them as well to be more friendly with your python users

These are more or less the bunch of things I'm aware of, but to me they foreshadow that there may be even something else that I'm not able to focus right now.

felixhekhorn commented 3 years ago

since rust it's compiled it may be not available on some platforms

however, I think in that case you should definitely throw an Exception yourself as the code will eventually result in a class-not-found-Exception anyhow and this is less verbose

cschwan commented 3 years ago

OK. So I'd suggest we remove the additional Rust layer and use the PyO3 macros conditionally, as in #[cfg_attr(feature="python", pymethods)], but keep the Python layer.

alecandido commented 3 years ago

Agreed, next week I'll make an attempt, but maybe in another branch (then we can merge that one in this, and everything into master)

codecov[bot] commented 3 years ago

Codecov Report

Merging #51 (c07ec12) into master (7be4973) will decrease coverage by 14.45%. The diff coverage is 0.87%.

:exclamation: Current head c07ec12 differs from pull request most recent head 3d8955b. Consider uploading reports for the commit 3d8955b to get more accurate results

@@             Coverage Diff             @@
##           master      #51       +/-   ##
===========================================
- Coverage   81.92%   67.46%   -14.46%     
===========================================
  Files          11       18        +7     
  Lines        1582     1921      +339     
===========================================
  Hits         1296     1296               
- Misses        286      625      +339

Impacted Files	Coverage Δ
pineappl/src/bin.rs	`88.05% <ø> (ø)`
pineappl/src/fk_table.rs	`0.00% <0.00%> (ø)`
pineappl/src/grid.rs	`41.83% <0.00%> (-18.31%)`	:arrow_down:
pineappl/src/import_only_subgrid.rs	`86.98% <ø> (ø)`
pineappl/src/lagrange_subgrid.rs	`84.61% <ø> (ø)`
pineappl/src/ntuple_subgrid.rs	`100.00% <ø> (ø)`
pineappl/src/sparse_array3.rs	`95.37% <ø> (ø)`
pineappl/src/subgrid.rs	`97.33% <ø> (ø)`
pineappl_py/src/bin.rs	`0.00% <0.00%> (ø)`
pineappl_py/src/fk_table.rs	`0.00% <0.00%> (ø)`
... and 12 more

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 7be4973...3d8955b. Read the comment docs.

cschwan commented 3 years ago

Thanks for the last few commits @AleCandido , in particular the change from the nested Vecs to Array5 makes the code much more readable IMO.

I saw that you've also fixed a few problems in the case of two PDFs. Did you rerun the test for the DY grid I sent you?

alecandido commented 3 years ago

Most likely yes, if I'm not confused about the grid you are speaking of. Have you had a look to N3PDF/pineko#5 ?

cschwan commented 3 years ago

Yes, I saw your comments but did you redo the DY test (ATLASZHIGHMASS49FB)? Did it improve? If I recall the test that I saw last produced numbers that were completely off.

alecandido commented 3 years ago

If I’m not wrong should be the one I showed in pineko and was taking 21h. Maybe I can restrict to a single bin and it will finish in a reasonable amount of time, I don’t remember exactly the setup of the test we did, and we were able to complete “quickly”.

cschwan commented 3 years ago

Here's a TODO list for myself:

[ ] merge master into this branch (again)
[ ] add a PID check in convolute_eko (check that for all PIDs an EK operator is available, and implement fallback rules, for instance that pid = 0 is the gluon)
[ ] multiply all grids with the corresponding power of alphas and add all results together; only then apply the EK operator

@AleCandido @felixhekhorn Did I forget anything?

cschwan commented 3 years ago

@AleCandido @felixhekhorn can you please add the correct yaml files to the repository, so I can run the example programs?

alecandido commented 3 years ago

@cschwan done: N3PDF/pineko@00aa4e1b

cschwan commented 3 years ago

@AleCandido can you still run the example program? I get

$ python examples/import-yadism-yaml-orders.py mydis.yaml output.pineappl
Namespace(input_yaml='mydis.yaml', output_pineappl='output.pineappl')
Traceback (most recent call last):
  File "/home/cschwan/projects/pineappl/pineappl_py/examples/import-yadism-yaml-orders.py", line 98, in <module>
    make_pineappl(args.input_yaml, args.output_pineappl)
  File "/home/cschwan/projects/pineappl/pineappl_py/examples/import-yadism-yaml-orders.py", line 43, in make_pineappl
    grid = pineappl.grid.Grid(
TypeError: __init__() takes 2 positional arguments but 5 were given

which I don't quite understand...

alecandido commented 3 years ago

@cschwan maybe you are not running the correct thing: import-yadism-yaml-orders.py is the old version of the yadism.output.Output API for dumping a pineapplgrid. If you want to test that one you can check how it is used inside pineko/src/pineko/__init__.py.

If you want to test the combination instead, you should actually try to run pineko/src/pineko/__init__.py (and this we tested and it is still working, with the updated runcards).

cschwan commented 3 years ago

@AleCandido Thanks - I wasn't aware of the fact that PineAPPL is now used inside yadism in here: https://github.com/N3PDF/yadism/blob/ca5fe1b59d8df300ce89b1febed3f6af0bb10c2a/src/yadism/output.py#L104

alecandido commented 3 years ago

Yes, and it was the literal (second) translation of your initial script, but since looks more like a script than a function it is going to be improved soon. If I remember you even introduced some other features in the API that will be useful for the DIS case.

NNPDF / pineappl

Python bindings with PyO3 #51

[pymethods]

[new]

Codecov Report