Python interface improvements by alecandido · Pull Request #74 · NNPDF/pineappl

alecandido · 2021-10-02T13:52:43Z

While working on stuffs that make use of python interface, new needs arose. Thus, there is room to add some improvements.

Following a list of possible ones:

a Grid.key_values() method see Run scripts with python pinecards#104 (comment)
~~a function close to merge subcommand see Run scripts with python pinecards#104 (comment)~~
~~move pineappl_cli::helpers::convolute() in main crate~~ moved to Fix Grid::convolute caching strategy #66
add python bindings for helpers::convolute()
maybe a function to generate order masks from something similar to "NLO QCD" or "NLO EW", see Allow to easily generate order masks #73
Python binding for Add Grid::write_lz4 method #80

alecandido · 2021-10-04T14:32:35Z

As discussed with @felixhekhorn we'll reserve Grid.__get_item__() and Grid.__set_item__() and Grid.__iter__() for managing subgrids, since Grid is a subgrids container in the first place (and a metadata container only afterwards).

alecandido · 2021-10-04T14:34:06Z

Most of the functions of CLI consists mainly in printing tables, something we don't need for the python interface (since we can handle objects, and it is comfortable to do it), nevertheless we are wondering if it's worth to bind pineappl_cli.helpers.

cschwan · 2021-10-04T15:09:34Z

Most of the functions of CLI consists mainly in printing tables, something we don't need for the python interface (since we can handle objects, and it is comfortable to do it), nevertheless we are wondering if it's worth to bind pineappl_cli.helpers.

If you find that helpful I'd rather suggest to import the functionality into PineAPPL and remove the corresponding helper (and write a wrapper to the new function in PineAPPL). I'm definitely going to do that for the convolute function, which takes care of charge conjugation and different initial states; this also overlaps with #66. Let me know if you need more.

alecandido · 2021-10-04T15:17:48Z

Most of the functions of CLI consists mainly in printing tables, something we don't need for the python interface (since we can handle objects, and it is comfortable to do it), nevertheless we are wondering if it's worth to bind pineappl_cli.helpers.

If you find that helpful I'd rather suggest to import the functionality into PineAPPL and remove the corresponding helper (and write a wrapper to the new function in PineAPPL). I'm definitely going to do that for the convolute function, which takes care of charge conjugation and different initial states; this also overlaps with #66. Let me know if you need more.

I would say just convolute() (and consequently SCALES_VECTOR). I can do here or we can do separately, that's fine.

You can move yourself if you wish, or I can move it, but: where should I place? and how should I call it? (my guess would be to put it in grid.rs as a separate functions, i.e. not inside the impl block, but I don't have any good name other than convolute).

cschwan · 2021-10-04T15:24:57Z

Most of the functions of CLI consists mainly in printing tables, something we don't need for the python interface (since we can handle objects, and it is comfortable to do it), nevertheless we are wondering if it's worth to bind pineappl_cli.helpers.

If you find that helpful I'd rather suggest to import the functionality into PineAPPL and remove the corresponding helper (and write a wrapper to the new function in PineAPPL). I'm definitely going to do that for the convolute function, which takes care of charge conjugation and different initial states; this also overlaps with #66. Let me know if you need more.

I would say just convolute() (and consequently SCALES_VECTOR). I can do here or we can do separately, that's fine.

You can move yourself if you wish, or I can move it, but: where should I place? and how should I call it? (my guess would be to put it in grid.rs as a separate functions, i.e. not inside the impl block, but I don't have any good name other than convolute).

My current idea is that instead of passing three functions (xfx1, xfx2 and alphas) you pass a single PdfCache object (for which I have yet to write the struct). You can then easily cache PDFs, even among several convolute calls, which will speed up a few of the subcommands.

alecandido · 2021-10-04T15:26:25Z

Added syntactic sugar to get and set subgrids, but the get function is not really working well.

The following code:

import pineappl
g = pineappl.grid.Grid.read("dyaa.pineappl")
s = g[0, 0, 0]
g[0, 1, 0] = s

(with dyaa.pineappl the grid generated from the example) raises with the following error

AttributeError                            Traceback (most recent call last)
<ipython-input-5-346819bef8ca> in <module>
----> 1 g[0, 1, 0] = s

~/Projects/N3PDF/pineappl/pineappl_py/env/lib/python3.9/site-packages/pineappl/grid.py in __setitem__(self, key, subgrid)
    140             raise ValueError("A tuple with `(order, bin, lumi)` is required as key.")
    141
--> 142         self.set_subgrid(*key, subgrid)
    143
    144     def set_remapper(self, remapper):

~/Projects/N3PDF/pineappl/pineappl_py/env/lib/python3.9/site-packages/pineappl/grid.py in set_subgrid(self, order, bin_, lumi, subgrid)
    122                 subgrid content
    123         """
--> 124         self.raw.set_subgrid(order, bin_, lumi, subgrid.into())
    125
    126     def __setitem__(self, key, subgrid):

AttributeError: 'builtins.PySubgridEnum' object has no attribute 'into'

Essentially the error is due to the subgrid not having been recast to any specific subgrid type when retrieved, so the subgrid.into() call is unneeded. I believe it should be recast to a meaningful type to be able to do operations, but since I don't know which one should be maybe that's a hint I'm not thinking to the correct solution.

cschwan · 2021-10-04T15:31:25Z

As discussed with @felixhekhorn we'll reserve Grid.__get_item__() and Grid.__set_item__() and Grid.__iter__() for managing subgrids, since Grid is a subgrids container in the first place (and a metadata container only afterwards).

Do __get_item__ and others significantly improve the usability of Grid? I'm a bit sceptical here; I'd actually like to discourage the use of subgrids, because everything should be done using convolute, convolute_subgrid and convolute_eko. This doesn't always work (for instance in yadism), but that should be the aim.

codecov · 2021-10-04T15:32:49Z

Codecov Report

Merging #74 (714b559) into master (c737cba) will increase coverage by 10.04%.
The diff coverage is 88.46%.

@@             Coverage Diff             @@
##           master      #74       +/-   ##
===========================================
+ Coverage   67.65%   77.69%   +10.04%     
===========================================
  Files          13       14        +1     
  Lines        2226     2049      -177     
===========================================
+ Hits         1506     1592       +86     
+ Misses        720      457      -263

Impacted Files	Coverage Δ
pineappl/src/fk_table.rs	`38.96% <ø> (+2.37%)`	⬆️
pineappl/src/lumi.rs	`70.87% <ø> (+63.18%)`	⬆️
pineappl/tests/drell_yan_lo.rs	`100.00% <ø> (ø)`
pineappl/src/grid.rs	`72.57% <88.46%> (+11.72%)`	⬆️
pineappl/src/subgrid.rs	`96.87% <0.00%> (-0.46%)`	⬇️
pineappl_cli/src/main.rs	`0.00% <0.00%> (ø)`
pineappl/src/bin.rs	`88.19% <0.00%> (+0.54%)`	⬆️
pineappl/src/lagrange_subgrid.rs	`87.13% <0.00%> (+2.52%)`	⬆️
pineappl/src/empty_subgrid.rs	`100.00% <0.00%> (+5.88%)`	⬆️
... and 3 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 c737cba...714b559. Read the comment docs.

alecandido · 2021-10-04T15:39:24Z

Do __get_item__ and others significantly improve the usability of Grid? I'm a bit sceptical here; I'd actually like to discourage the use of subgrids, because everything should be done using convolute, convolute_subgrid and convolute_eko. This doesn't always work (for instance in yadism), but that should be the aim.

Ok, this might be a point. Nevertheless, I wonder if giving write access without read access is a good idea (about __setitem__ instead is just for building abstractions, in python it is useful to build on existing abstractions, and if a Grid is a subgrid containers you expect to be able to access them, set them and iterate over them, doing it with standard functions helps the user to rely on known abstractions).

In case I can just revert and remove Grid.subgrid().

alecandido · 2021-10-04T15:41:44Z

Instead I'm running in more serious troubles with Grid.merge().

We decided to keep Grid non-clonable, and I agree it is a good idea. But for Grid.merge() we need to pass a second grid, and it looks like it doesn't like:

error[E0277]: the trait bound `PyGrid: Clone` is not satisfied
   --> src/grid.rs:345:40
    |
345 |     pub fn merge(&mut self, mut other: Self) {
    |                                        ^^^^ the trait `Clone` is not implemented for `PyGrid`
    |
    = note: required because of the requirements on the impl of `pyo3::FromPyObject<'_>` for `PyGrid`

Any suggestion?

P.S.: PyGrid is not clonable because it contains a Grid, that as said above it's not clonable

cschwan · 2021-10-04T16:53:05Z

Instead I'm running in more serious troubles with Grid.merge().

We decided to keep Grid non-clonable, and I agree it is a good idea. But for Grid.merge() we need to pass a second grid, and it looks like it doesn't like:
error[E0277]: the trait bound `PyGrid: Clone` is not satisfied
   --> src/grid.rs:345:40
    |
345 |     pub fn merge(&mut self, mut other: Self) {
    |                                        ^^^^ the trait `Clone` is not implemented for `PyGrid`
    |
    = note: required because of the requirements on the impl of `pyo3::FromPyObject<'_>` for `PyGrid`
Any suggestion?

P.S.: PyGrid is not clonable because it contains a Grid, that as said above it's not clonable

That's a tough one, let me think about it.

felixhekhorn · 2021-10-05T08:32:43Z

Mmm, I think the only solution (if we want to keep Grid non-clonable) is to provide a function merge_from_file(Grid, path) and then read the Grid in Rust ...

cschwan · 2021-10-18T13:32:20Z

Mmm, I think the only solution (if we want to keep Grid non-clonable) is to provide a function merge_from_file(Grid, path) and then read the Grid in Rust ...

Yes, I agree this is how we should do it!

alecandido · 2021-10-18T13:55:50Z

Mmm, I think the only solution (if we want to keep Grid non-clonable) is to provide a function merge_from_file(Grid, path) and then read the Grid in Rust ...

Yes, I agree this is how we should do it!

Then I'm going to implement it immediately

This commit also removes the use of `anyhow`, for which instead `thiserror` is used. Furthermore, it adds the method `Grid::write_lz4`, which writes a compressed grid. This method is used in the CLI whenever a file is written that has an extension 'lz4'.

cschwan · 2021-10-26T12:46:01Z

The last item is implemented in commit dfdcdcc for the Rust API and in the CLI. Whenever you run pineappl with a subcommand that writes a file, giving the file the extension .lz4 also let's the CLI compress it.

alecandido · 2021-10-26T15:41:39Z

Re #74 (comment)

@cschwan tested with pineko together with @felixhekhorn, it required a very tiny fix (b1bba7e) but it's now working.

alecandido · 2021-10-27T09:31:07Z

Sorry to have waited for so long, but now this PR should address even point 4 with a4bf1f2, it was very simple.

Closes #80

cschwan · 2021-10-27T09:40:25Z

Commit 714b559 should let you select a subset of orders in a Grid when generating an FkTable with Grid::convolute_eko.

cschwan · 2021-10-27T13:16:04Z

@alecandido @felixhekhorn can we merge this?

alecandido · 2021-10-27T13:49:30Z

I reviewed just now, and the only thing I could spot is really the migration of convolute_subgrid, but there is a TODO and we can do on master.

Then let's merge, I would say.

cschwan · 2021-10-27T13:59:57Z

@alecandido @felixhekhorn do you need another release for the Python interface to be uploaded? If so, I'm going to release it as 0.5.0-beta.2.

alecandido · 2021-10-27T14:04:45Z

Yes, that would be nice. Then I'm going to update dependencies in runcards, pineko. and yadism.

felixhekhorn · 2021-10-27T14:08:09Z

Not necessarily, but a release would make things easier and since the last we added several new features ...
by the way, are you're sure about the naming? shouldn't it be 0.5.0b2? see https://pypi.org/project/poetry/#history

alecandido · 2021-10-27T14:22:16Z

I believe there is no official way of naming alpha, beta, and release_candidate (for sure it's not specified in semver). Just be consistent with one scheme.

cschwan · 2021-10-27T14:26:06Z

Yes, as far as semver is concerned both are OK (I think).

alecandido · 2021-10-27T14:29:01Z

Ok, I just checked and the answer is definitely yes:

A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version. Identifiers MUST comprise only ASCII alphanumerics and hyphens [0-9A-Za-z-]. Identifiers MUST NOT be empty. Numeric identifiers MUST NOT include leading zeroes. Pre-release versions have a lower precedence than the associated normal version. A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version. Examples: 1.0.0-alpha, 1.0.0-alpha.1, 1.0.0-0.3.7, 1.0.0-x.7.z.92, 1.0.0-x-y-z.–.

https://semver.org/#spec-item-9

or even more: @cschwan is more correct then poetry, that is not using the dash (but even the official CPython is omitting the dash...)

cschwan · 2021-10-27T15:22:14Z

@alecandido I released v0.5.0-beta.2, but building the documentation fails and I can't reproduce the error: https://readthedocs.org/projects/pineappl/builds/15107369/

alecandido · 2021-10-27T17:15:53Z

Sorry not to have replied before, I'm glad that you already solved the issue!

Add key_values to grid interface

da1b703

alecandido changed the base branch from pyo3-simplify to master October 4, 2021 14:30

Add get subgrid and syntactic sugar

c323ab2

alecandido mentioned this pull request Oct 15, 2021

Complete runpy package NNPDF/pinecards#108

Closed

9 tasks

cschwan mentioned this pull request Oct 18, 2021

Support evolution basis FkTables #75

Merged

alecandido and others added 11 commits October 18, 2021 16:00

Add separe target pids for eko convolution

982168e

Sync python interface with pids update

d0c6493

Allow additional metadata in FkTable generation

18bee8f

Introduce new Eko related structs

32d0b51

Improve documentation strings

7b78670

Add temporary axes test

558db3e

Add broken test for convolute_eko

b527d9d

Fix convolute_eko test

36bf0c6

Remove the too flexible additional metadata from EkoInfo

0533666

Fix bug in conv_eko, expose optimize

4c5f2e5

Update lumi_id_types docstring

2cef20e

cschwan added 7 commits October 26, 2021 11:39

Fix documentation of CAPI convolute functions

cdefdbe

Rename Grid::convolute2 to Grid::convolute

8352253

Remove order_mask parameter for FkTable

f06d629

Fix broken test

f64cec5

Fix a few clippy warnings

2ad8571

Add TODO item

9c7215e

cschwan and others added 2 commits October 26, 2021 14:46

Add missing dev-dependency

9445166

Fix leftovers of convolute migration

b1bba7e

Expose write_lz4 within python API

a4bf1f2

alecandido mentioned this pull request Oct 27, 2021

Allow to easily generate order masks #73

Closed

alecandido linked an issue Oct 27, 2021 that may be closed by this pull request

Add Grid::write_lz4 method #80

Closed

alecandido marked this pull request as ready for review October 27, 2021 09:35

Add order_mask param to Grid::convolute_eko

714b559

cschwan merged commit 714b559 into master Oct 27, 2021

cschwan deleted the py-improved branch October 27, 2021 13:54

Conversation

alecandido commented Oct 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecandido commented Oct 4, 2021

Uh oh!

alecandido commented Oct 4, 2021

Uh oh!

cschwan commented Oct 4, 2021

Uh oh!

alecandido commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschwan commented Oct 4, 2021

Uh oh!

alecandido commented Oct 4, 2021

Uh oh!

cschwan commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alecandido commented Oct 4, 2021

Uh oh!

alecandido commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschwan commented Oct 4, 2021

Uh oh!

felixhekhorn commented Oct 5, 2021

Uh oh!

cschwan commented Oct 18, 2021

Uh oh!

alecandido commented Oct 18, 2021

Uh oh!

cschwan commented Oct 26, 2021

Uh oh!

alecandido commented Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecandido commented Oct 27, 2021

Uh oh!

cschwan commented Oct 27, 2021

Uh oh!

cschwan commented Oct 27, 2021

Uh oh!

alecandido commented Oct 27, 2021

Uh oh!

cschwan commented Oct 27, 2021

Uh oh!

alecandido commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixhekhorn commented Oct 27, 2021

Uh oh!

alecandido commented Oct 27, 2021

Uh oh!

cschwan commented Oct 27, 2021

Uh oh!

alecandido commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschwan commented Oct 27, 2021

Uh oh!

alecandido commented Oct 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alecandido commented Oct 2, 2021 •

edited

Loading

alecandido commented Oct 4, 2021 •

edited

Loading

cschwan commented Oct 4, 2021 •

edited

Loading

codecov Bot commented Oct 4, 2021 •

edited

Loading

alecandido commented Oct 4, 2021 •

edited

Loading

alecandido commented Oct 26, 2021 •

edited

Loading

alecandido commented Oct 27, 2021 •

edited

Loading

alecandido commented Oct 27, 2021 •

edited

Loading