Scale ratios in `convolute_eko` by felixhekhorn · Pull Request #138 · NNPDF/pineappl

felixhekhorn · 2022-04-08T13:51:43Z

This should close #133

it got much more involved than I naively expected, since I even had to touch pineappl
this is untested at the moment sine I first have to write the pineko counter-part
moreover, currently test are broken for me locally: cargo test results in test result: FAILED. 24 passed; 59 failed; 1 ignored; 0 measured; 0 filtered out; finished in 114.04s, but some of this stuff is not coming from my changes, but they fail because "INPUT" != "input" - do I have to use Rust nightly?
The changes are non-trivial so if you can please start having a look to the changes

if I may suggest further:

short term goal: provide some unit test for convolute_eko (so I don't have to run manually and use pineko) (already mentioned by @alecandido )
mid term goal: split up convolute_eko - it is 370 non-trivial lines long ...

codecov · 2022-04-08T13:54:12Z

Codecov Report

Merging #138 (195f4d6) into master (e2a02b0) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   89.75%   89.80%   +0.04%     
==========================================
  Files          45       45              
  Lines        6826     6858      +32     
==========================================
+ Hits         6127     6159      +32     
  Misses        699      699

Flag	Coverage Δ
python	`80.64% <100.00%> (+0.85%)`	⬆️
rust	`89.93% <100.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pineappl/src/grid.rs	`87.87% <100.00%> (+0.09%)`	⬆️
pineappl_py/pineappl/grid.py	`73.33% <100.00%> (+1.59%)`	⬆️
pineappl_cli/src/plot.rs	`94.11% <0.00%> (-0.21%)`	⬇️
pineappl_cli/src/convolute.rs	`97.18% <0.00%> (+0.08%)`	⬆️
pineappl_cli/src/helpers.rs	`91.89% <0.00%> (+0.28%)`	⬆️
pineappl_cli/src/ops.rs	`91.01% <0.00%> (+1.82%)`	⬆️

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 e2a02b0...195f4d6. Read the comment docs.

alecandido · 2022-04-08T14:31:15Z

@felixhekhorn do you want an intermediate review? Or did you just forget to click "Ready for review" removing the "Draft" status?

alecandido · 2022-04-08T14:32:57Z

do I have to use Rust nightly?

The workflow not failing means that at least on nightly is working. @cschwan will reply more precisely.

However, which version of Rust do you have?

felixhekhorn · 2022-04-08T14:32:59Z

@felixhekhorn do you want an intermediate review?

yes, please!

Or did you just forget to click "Ready for review" removing the "Draft" status?

no, this was intentional - as said it is untested

felixhekhorn · 2022-04-08T14:34:35Z

However, which version of Rust do you have?

$ rustup  show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/felix/.rustup

stable-x86_64-unknown-linux-gnu (default)
rustc 1.58.1 (db9d1b20b 2022-01-20)

alecandido · 2022-04-08T14:37:16Z

rustc 1.58.1 (db9d1b20b 2022-01-20)

Before nightly there are at least 1.59 and 1.60...

alecandido · 2022-04-08T14:39:01Z

On 1.60 I pass all but tests/drell_yan_lo.rs.

alecandido · 2022-04-08T14:40:50Z

Consider that Rust policy for releases is: "a new one every 6 weeks". And there are three channels: stable, beta, and nightly.

Nightly is rather distant from where you are :)

alecandido

For the time being it's pretty straightforward. Apart of the increased documentation (that is good), there is just the introduction of mu_r in the internals, and exposure of xi in the interface.

I'd say it is fine :)

cschwan · 2022-04-08T15:30:37Z

On 1.60 I pass all but tests/drell_yan_lo.rs.

What's the problem with DY? If you rerun and it goes away that's LHAPDF's fault: https://gitlab.com/hepcedar/lhapdf/-/merge_requests/27.

alecandido · 2022-04-08T15:49:10Z

What's the problem with DY? If you rerun and it goes away that's LHAPDF's fault: https://gitlab.com/hepcedar/lhapdf/-/merge_requests/27.

Yes it is, but actually there are still a few failures:



failures:
    delete::tests::help
    ops::tests::cc1
    ops::tests::cc2
    ops::tests::help
    optimize::tests::help
    plot::tests::default
    plot::tests::subgrid_pull
    pull::tests::cl_90
    pull::tests::default
    pull::tests::limit
    sum::tests::bins
    sum::tests::help

test result: FAILED. 71 passed; 12 failed; 1 ignored; 0 measured; 0 filtered out; finished in 12.89s

Version of Rust for reproducibility:

❯ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/alessandro/.rustup

installed targets for active toolchain
--------------------------------------

wasm32-unknown-unknown
x86_64-unknown-linux-gnu

active toolchain
----------------

stable-x86_64-unknown-linux-gnu (default)
rustc 1.60.0 (7737e0b5c 2022-04-04)

felixhekhorn · 2022-04-08T15:53:07Z

On 1.60 I pass all but tests/drell_yan_lo.rs.

Mmm ... I'm still failing:

$ rustup show
Default host: x86_64-unknown-linux-gnu
rustup home:  /home/felix/.rustup

stable-x86_64-unknown-linux-gnu (default)
rustc 1.60.0 (7737e0b5c 2022-04-04)
$ cargo test
[...]
test result: FAILED. 24 passed; 59 failed; 1 ignored; 0 measured; 0 filtered out; finished in 158.41s

cschwan · 2022-04-08T15:54:05Z

@felixhekhorn what is [...]?

felixhekhorn · 2022-04-08T15:58:51Z

$ cargo test > test.log
   Compiling lhapdf v0.1.11
   Compiling pineappl v0.5.2 (/home/felix/Physik/N3PDF/PineAPPL/pineappl/pineappl)
   Compiling pineappl_cli v0.5.2 (/home/felix/Physik/N3PDF/PineAPPL/pineappl/pineappl_cli)
warning: unused import: `assert_fs::NamedTempFile`
   --> pineappl_cli/src/import.rs:161:9
    |
161 |     use assert_fs::NamedTempFile;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_imports)]` on by default

warning: `pineappl_cli` (bin "pineappl" test) generated 1 warning
    Finished test [unoptimized + debuginfo] target(s) in 5.31s
     Running unittests (target/debug/deps/pineappl-88ec2c506a9d5ffd)
[00:00:00] ██████████████████████████████████████████████████      18/18      - ETA: 00:00:00 
     Running tests/drell_yan_lo.rs (target/debug/deps/drell_yan_lo-a6da7d940ee971e1)
     Running unittests (target/debug/deps/pineappl_capi-69cbfbede568447a)
     Running unittests (target/debug/deps/pineappl-660e482cf49eda12)
error: test failed, to rerun pass '-p pineappl_cli --bin pineappl'

test.log

cschwan · 2022-04-08T16:03:47Z

@felixhekhorn I see. Before testing you must first compile the CLI with the same features and ideally same build flags, for instance:

cargo build && cargo test

in the main directory. I should document this somewhere.

felixhekhorn · 2022-04-08T16:17:50Z

@felixhekhorn I see. Before testing you must first compile the CLI with the same features and ideally same build flags, for instance:
cargo build && cargo test
in the main directory. I should document this somewhere.

I see - I was hoping that compiling the CLI would be enough (which at least at the beginning I did - I'm not sure for the last run though)

now, I'm down to two failures plot::tests::default,plot::tests::subgrid_pull (a subset of #138 (comment)) and one skipped

alecandido · 2022-04-08T16:26:49Z

I still have more:

failures:
    plot::tests::default
    plot::tests::subgrid_pull
    pull::tests::cl_90
    pull::tests::default
    pull::tests::limit

test result: FAILED. 78 passed; 5 failed; 1 ignored; 0 measured; 0 filtered out; finished in 14.73s

cschwan · 2022-04-09T10:57:20Z

@felixhekhorn @alecandido please upload your (updated) the test logs, I'm not telepathic yet 😄.

alecandido · 2022-04-09T16:43:37Z

Sorry, it was trivial, solved by myself.

I was missing the PDF set, so the actual solution has been:

❯ lhapdf install NNPDF40_nnlo_as_01180

P.S.: now there is only one ignored: test tests::help ... ignored, but I guess this is intentional (it seems hard to be ignored for any other reason, unless it can detect something missing)

cschwan · 2022-04-10T09:09:03Z

P.S.: now there is only one ignored: test tests::help ... ignored, but I guess this is intentional (it seems hard to be ignored for any other reason, unless it can detect something missing)

Yes, this is intentional, because the global help command prints the CLI's version number, which would change every commit. That's something I have to fix.

cschwan · 2022-04-10T09:09:45Z

Sorry, it was trivial, solved by myself.

I was missing the PDF set, so the actual solution has been:
❯ lhapdf install NNPDF40_nnlo_as_01180

@felixhekhorn does that also solve your failed tests?

felixhekhorn · 2022-04-13T09:24:30Z

@felixhekhorn does that also solve your failed tests?

Unfortunately, no:

$ lhapdf ls --installed | grep NNPDF40_
NNPDF40_lo_as_01180
NNPDF40_nlo_as_01180
NNPDF40_nnlo_as_01180
$ cargo test -p pineappl_cli --bin pineappl > test.log
warning: unused import: `assert_fs::NamedTempFile`
   --> pineappl_cli/src/import.rs:161:9
    |
161 |     use assert_fs::NamedTempFile;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(unused_imports)]` on by default

warning: `pineappl_cli` (bin "pineappl" test) generated 1 warning
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests (target/debug/deps/pineappl-660e482cf49eda12)
error: test failed, to rerun pass '-p pineappl_cli --bin pineappl'

test.log

I'm still on LHAPDF v6.3.0 though, do you think that might be a problem?

cschwan · 2022-04-13T09:51:19Z

@felixhekhorn thanks. This is a numerical problem, and caused by the fact that I'm printing the convoluted numbers with too many digits into the plot script. I think this is a good point to reduce them to 7 digits or even less.

cschwan · 2022-04-13T12:52:30Z

@felixhekhorn could you please try again with the most recent master?

felixhekhorn · 2022-04-14T08:47:25Z

Not yet, still 1 test failing ... test.log

diff: 
│   --- 	orig
│   +++ 	var
│   @@ -8,3 +8,3 @@
│   -x1 = np.array([1.0000000e0, 1.0000000e0, 1.0000000e0,
+x1 = np.array([9.3094408e-1, 9.3094408e-1, 9.3094408e-1,

there is something truly different ...

cschwan · 2022-04-14T09:39:53Z

This is likely yet another numerical problem. Could you try master again? It'll likely fail, but I'm interested in the test log. Don't forget to rebuild the CLI first!

felixhekhorn · 2022-04-14T09:51:47Z

$ cargo build && cargo test -p pineappl_cli --bin pineappl > test.log -> test.log

yes, it is still failing ...

cschwan · 2022-04-14T13:42:37Z

@felixhekhorn that's interesting, when I use LHAPDF 6.3.0 I can reproduce your failing test...

felixhekhorn · 2022-04-14T15:06:45Z

(Note that this discussion is completely off-topic ;-) but okay already too late ... )

so I tried to upgrade to LHAPDF 6.4.0, but now I get a whole new bunch of new errors which I suspect are all related to LHAPDF: test.log

still, I can run lhapdf ls in bash and I can run import lhapdf in ipython - so something has worked

Moreover, looking at the ChangeLog I can't see an obvious change of interpolation (which we are seeing) (except maybe? the change on 2021-09-16)

felixhekhorn · 2022-04-14T15:13:31Z

And outside the test I can run e.g. $ pineappl --silence-lhapdf convolute pineappl_cli/data/LHCB_WP_7TEV.pineappl.lz4 NNPDF31_nlo_as_0118_luxqed

alecandido · 2022-04-14T23:10:21Z

Just to be sure, try to cancel your target/ folder (or clone the repo anew) and retry.

Of course you tried to check with the CLI and Python package the LHAPDF version and they are actually telling 6.4.0, is it correct?

cschwan · 2022-04-15T15:30:40Z

On-topic: looks good to me. At this point we really need a test case...

cschwan · 2022-04-19T09:35:59Z

@felixhekhorn I'm OK with you merging this into master if you like, tests can be added later on, as part of #108 or #122 for instance.

felixhekhorn · 2022-07-08T08:46:57Z

@cschwan although scheme B is not fully solved yet - can we merge this branch?

Expose scale ratios in convolute_eko

1ad4f22

felixhekhorn added the enhancement New feature or request label Apr 8, 2022

felixhekhorn requested review from alecandido and cschwan April 8, 2022 13:51

alecandido reviewed Apr 8, 2022

View reviewed changes

Comment thread pineappl/src/grid.rs

felixhekhorn mentioned this pull request Apr 8, 2022

MHOU implement scheme B+C NNPDF/pineko#6

Closed

3 tasks

felixhekhorn mentioned this pull request Apr 8, 2022

Composite Output NNPDF/eko#105

Merged

14 tasks

felixhekhorn mentioned this pull request Apr 8, 2022

Stop propagating alpha_s in output NNPDF/eko#113

Closed

Merge branch 'master' into feature/sv-evolution

84a1148

Merge branch 'master' into feature/sv-evolution

6168dd8

cschwan approved these changes Apr 19, 2022

View reviewed changes

cschwan mentioned this pull request Apr 19, 2022

PineAPPL CLI tests are sensitive to changes in the LHAPDF version #139

Closed

Extract unique mur2_grid in axes()

387be87

alecandido reviewed May 11, 2022

View reviewed changes

Comment thread pineappl/src/grid.rs Outdated

felixhekhorn added 3 commits May 12, 2022 12:35

Partially replace unwrap_or_else with just unwrap

fb60a7e

Closes #136

42449a7

Reformat pineappl_py

40715b0

alecandido reviewed Jun 1, 2022

View reviewed changes

Comment thread pineappl_py/src/lib.rs

Merge branch 'master' into feature/sv-evolution

2a1b986

felixhekhorn mentioned this pull request Jun 17, 2022

Allow pineko to check if the grid contains SV NNPDF/pineko#24

Merged

Fix typo

195f4d6

felixhekhorn marked this pull request as ready for review July 8, 2022 08:45

cschwan merged commit 30b3f39 into master Jul 8, 2022

cschwan deleted the feature/sv-evolution branch July 8, 2022 09:22

Conversation

felixhekhorn commented Apr 8, 2022

Uh oh!

codecov Bot commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

felixhekhorn commented Apr 8, 2022

Uh oh!

felixhekhorn commented Apr 8, 2022

Uh oh!

alecandido commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

alecandido left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cschwan commented Apr 8, 2022

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

felixhekhorn commented Apr 8, 2022

Uh oh!

cschwan commented Apr 8, 2022

Uh oh!

felixhekhorn commented Apr 8, 2022

Uh oh!

cschwan commented Apr 8, 2022

Uh oh!

felixhekhorn commented Apr 8, 2022

Uh oh!

alecandido commented Apr 8, 2022

Uh oh!

cschwan commented Apr 9, 2022

Uh oh!

alecandido commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschwan commented Apr 10, 2022

Uh oh!

cschwan commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixhekhorn commented Apr 13, 2022

Uh oh!

cschwan commented Apr 13, 2022

Uh oh!

cschwan commented Apr 13, 2022

Uh oh!

felixhekhorn commented Apr 14, 2022

Uh oh!

cschwan commented Apr 14, 2022

Uh oh!

felixhekhorn commented Apr 14, 2022

Uh oh!

cschwan commented Apr 14, 2022

Uh oh!

felixhekhorn commented Apr 14, 2022

Uh oh!

felixhekhorn commented Apr 14, 2022

Uh oh!

alecandido commented Apr 14, 2022

Uh oh!

cschwan commented Apr 15, 2022

Uh oh!

cschwan commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixhekhorn commented Jul 8, 2022

codecov Bot commented Apr 8, 2022 •

edited

Loading

alecandido commented Apr 8, 2022 •

edited

Loading

alecandido commented Apr 9, 2022 •

edited

Loading

cschwan commented Apr 10, 2022 •

edited

Loading

cschwan commented Apr 19, 2022 •

edited

Loading