felixhekhorn commented 2 years ago

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[bot] commented 2 years ago

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%)`	:arrow_up:
rust	`89.93% <100.00%> (+0.03%)`	:arrow_up:

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%)`	:arrow_up:
pineappl_py/pineappl/grid.py	`73.33% <100.00%> (+1.59%)`	:arrow_up:
pineappl_cli/src/plot.rs	`94.11% <0.00%> (-0.21%)`	:arrow_down:
pineappl_cli/src/convolute.rs	`97.18% <0.00%> (+0.08%)`	:arrow_up:
pineappl_cli/src/helpers.rs	`91.89% <0.00%> (+0.28%)`	:arrow_up:
pineappl_cli/src/ops.rs	`91.01% <0.00%> (+1.82%)`	: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 e2a02b0...195f4d6. Read the comment docs.

alecandido commented 2 years ago

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

alecandido commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

rustc 1.58.1 (db9d1b20b 2022-01-20)

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

alecandido commented 2 years ago

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

alecandido commented 2 years ago

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 :)

cschwan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@felixhekhorn what is [...]?

felixhekhorn commented 2 years ago

$ 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 commented 2 years ago

@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 commented 2 years ago

@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 https://github.com/N3PDF/pineappl/pull/138#issuecomment-1093025777) and one skipped

alecandido commented 2 years ago

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 commented 2 years ago

@felixhekhorn @AleCandido please upload your (updated) the test logs, I'm not telepathic yet :smile:.

alecandido commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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

felixhekhorn commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

yes, it is still failing ...

cschwan commented 2 years ago

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

felixhekhorn commented 2 years ago

(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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

cschwan commented 2 years ago

@felixhekhorn I'm OK with you merging this into master if you like, tests can be added later on, as part of https://github.com/N3PDF/pineappl/issues/108 or https://github.com/N3PDF/pineappl/issues/122 for instance.

felixhekhorn commented 2 years ago

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

NNPDF / pineappl

Scale ratios in `convolute_eko` #138

Codecov Report