Allow pineko to check if the grid contains SV by andreab1997 · Pull Request #24 · NNPDF/pineko

andreab1997 · 2022-06-13T12:34:32Z

We want to allow pineko to check if scale variations are available in a grid if we are computing a scale-varied FK table.

codecov · 2022-06-13T12:41:30Z

Codecov Report

Merging #24 (3010f8c) into main (58715ad) will increase coverage by 0.75%.
The diff coverage is 47.54%.

@@            Coverage Diff             @@
##             main      #24      +/-   ##
==========================================
+ Coverage   39.24%   40.00%   +0.75%     
==========================================
  Files          15       15              
  Lines         479      520      +41     
==========================================
+ Hits          188      208      +20     
- Misses        291      312      +21

Flag	Coverage Δ
unittests	`40.00% <47.54%> (+0.75%)`	⬆️

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

Impacted Files	Coverage Δ
src/pineko/cli/__init__.py	`100.00% <ø> (ø)`
src/pineko/evolve.py	`30.00% <0.00%> (+1.15%)`	⬆️
src/pineko/theory.py	`20.12% <18.18%> (-0.40%)`	⬇️
src/pineko/cli/check.py	`46.15% <38.46%> (-17.01%)`	⬇️
src/pineko/cli/convolute.py	`77.27% <71.42%> (-6.07%)`	⬇️
src/pineko/check.py	`57.14% <100.00%> (+32.14%)`	⬆️

felixhekhorn

this PR should sit on top of #19 since even scheme C is not available in master
I assume this will also trigger more minor issues (e.g. the numpy doc style ...)

andreab1997 · 2022-06-17T11:56:14Z

Scale Variations #19

Should I rebase this on #19 then? @felixhekhorn

felixhekhorn · 2022-06-17T12:19:26Z

Should I rebase this on #19 then? @felixhekhorn

yes, please

andreab1997 · 2022-06-17T12:50:36Z

@felixhekhorn I have done but now there are some problems on stuffs that I have not touched. Do you know why?

felixhekhorn · 2022-06-17T13:39:35Z

@felixhekhorn I have done but now there are some problems on stuffs that I have not touched. Do you know why?

you mean here in the workflow? keep in mind that this only works together with NNPDF/pineappl#138 so an unpublished version - check locally instead

felixhekhorn · 2022-07-05T12:23:11Z

Could you please do me a favour and also adjust the arguments of https://github.com/N3PDF/pineko/blob/b4dc401e4feef5cffc4b137081c246b66c8ea8d8/src/pineko/evolve.py#L79 such that in TheoryBuilder.fk we open the grid only once?

andreab1997 · 2022-07-05T12:34:35Z

Could you please do me a favour and also adjust the arguments of

https://github.com/N3PDF/pineko/blob/b4dc401e4feef5cffc4b137081c246b66c8ea8d8/src/pineko/evolve.py#L79

such that in TheoryBuilder.fk we open the grid only once?

So you want that also evolve_grid takes the grid itself as argument and not the path, right?

andreab1997 · 2022-07-05T14:09:44Z

@felixhekhorn Ok but then we need to give up the fancy print with the grid path because we don't know that information anymore. Is it ok for you?
https://github.com/N3PDF/pineko/blob/e12c6482a32c7c8f902bfa2d2504e4bfe451fd19/src/pineko/evolve.py#L100

felixhekhorn · 2022-07-05T16:25:21Z

@felixhekhorn Ok but then we need to give up the fancy print with the grid path because we don't know that information anymore. Is it ok for you?

https://github.com/N3PDF/pineko/blob/e12c6482a32c7c8f902bfa2d2504e4bfe451fd19/src/pineko/evolve.py#L100

Mmm, actually, I like that one because it is telling me what is happening ... I guess we can lift it to the calling functions ... maybe on the "theory" side we can even adjust it a bit (since there the key ingredient is actually repeated 3 times ...)

andreab1997 · 2022-07-09T13:32:09Z

@andreab1997 please try to rebase on feature/sv, I'm getting conflicts...

I should have solved both the problems (the conflicts and the workflow failing)

alecandido

Few improvements, I'll give a tiny contribution as well and then we can merge.

alecandido · 2022-07-09T13:13:27Z

        )
+        # check if grid contains SV if theory is requesting them
+        xir = tcard["XIR"]
+        xif = tcard["XIF"]


@felixhekhorn didn't we have another variable, e.g. in yadism. In order to compute scale variations, even when we are not given a specific value?

yes, we have there "activateFact" (or similar name) in order to decide whether to compute the SV grids - however for pineko this is not sufficient since we need to collapse on the actual value - or what were you thinking?

Sorry, so it is just the first part of the comment to be referred to the code below.

Maybe I'd just move it: first # check if sv are requested then # in case of sv, check they are available in the grid.

you mean just the comments right? because the code is already doing that: first it checks if the theory is asking for scale-variations and then it checks whether they are available in the gird.

felixhekhorn · 2022-07-11T13:38:58Z

I hope github is joking

felixhekhorn · 2022-07-11T13:40:12Z

this PR is next in line ... open issues are

andreab1997 · 2022-07-12T10:21:53Z

for this, should I add a couple of tests or we add them in a separate PR? because codecov is complaining....

alecandido · 2022-07-12T11:29:17Z

@andreab1997 can you test the new refactored version of check subcommand, please? 975b8fe

andreab1997 · 2022-07-12T11:37:34Z

@andreab1997 can you test the new refactored version of check subcommand, please? 975b8fe

sure

andreab1997 · 2022-07-12T12:52:02Z

@andreab1997 can you test the new refactored version of check subcommand, please? 975b8fe

I tried that and it works. However I believe we should not add unit tests in this PR but rather in #25 mainly because we need actual grids to test the cli (as are provided by #25). So I think we can merge this now

felixhekhorn · 2022-07-12T12:56:33Z

I tried that and it works. However I believe we should not add unit tests in this PR but rather in #25 mainly because we need actual grids to test the cli (as are provided by #25). So I think we can merge this now

let's close this PR and also #25, #27, #28 - and have a restart from a new main branch; just try to keep track of some minor comments such as this one

andreab1997 · 2022-07-12T13:01:39Z

I tried that and it works. However I believe we should not add unit tests in this PR but rather in #25 mainly because we need actual grids to test the cli (as are provided by #25). So I think we can merge this now

let's close this PR and also #25, #27, #28 - and have a restart from a new main branch; just try to keep track of some minor comments such as this one

Yes, I am asking the review only for the test part, then I believe we can merge

andreab1997 · 2022-07-12T15:01:51Z

Can I merge this? @felixhekhorn @alecandido

andreab1997 added 2 commits June 13, 2022 14:28

First implementation

ff57aab

Fixing

c13474d

alecandido reviewed Jun 13, 2022

View reviewed changes

Comment thread src/pineko/check.py Outdated

andreab1997 added 4 commits June 13, 2022 15:04

Put scalevar parameters as arguments

f7f3e06

Fixed init of cli

02ee40d

Added SV check as default when computing fks

4fad303

Fixing

7713ff0

andreab1997 requested review from alecandido and felixhekhorn June 16, 2022 12:21

felixhekhorn requested changes Jun 16, 2022

View reviewed changes

Comment thread src/pineko/check.py Outdated

Comment thread src/pineko/check.py Outdated

Comment thread src/pineko/cli/check_scalevar.py Outdated

Comment thread src/pineko/cli/check_scalevar.py Outdated

Comment thread src/pineko/check.py Outdated

felixhekhorn added the enhancement New feature or request label Jun 16, 2022

andreab1997 added 2 commits June 17, 2022 13:50

Splitted funcs, put if in theory and change cli

b1598f6

Fixing

c7987f8

andreab1997 requested a review from felixhekhorn June 17, 2022 12:17

andreab1997 added 3 commits June 17, 2022 14:44

Splitted funcs, put if in theory and change cli

9fdd291

Fixing

8322198

Merge branch 'sv_check' of github.com:N3PDF/pineko into sv_check

f2eecce

felixhekhorn changed the base branch from main to feature/sv June 17, 2022 13:39

felixhekhorn requested changes Jul 5, 2022

View reviewed changes

Comment thread src/pineko/check.py Outdated

Comment thread src/pineko/cli/check.py Outdated

Change argument of the function and put tocheck as argument

b4dc401

put the grid as argument of evolve

8c14399

andreab1997 added 2 commits July 9, 2022 15:24

Fixed import of pineappl

381426e

Merge branch 'sv_check' of github.com:N3PDF/pineko into sv_check

6cec81c

alecandido requested changes Jul 9, 2022

View reviewed changes

Minor changes

b88a051

alecandido requested changes Jul 10, 2022

View reviewed changes

Comment thread debug.py Outdated

Comment thread eko_debug.py Outdated

Comment thread src/pineko/check.py Outdated

Comment thread yadism_debug.py Outdated

andreab1997 and others added 3 commits July 11, 2022 11:23

Change names of funcs

7c30efe

Fixed theory

261a2c7

Merge branch 'feature/sv' into sv_check

776af28

delete-merged-branch Bot deleted the branch main July 11, 2022 13:36

felixhekhorn changed the base branch from feature/sv to main July 11, 2022 13:37

andreab1997 mentioned this pull request Jul 12, 2022

Change max_as during FONLL-B DIS fk calculation #27

Merged

andreab1997 and others added 2 commits July 12, 2022 13:24

Added relevant tests

a3bc157

Refactor check subcommand

975b8fe

andreab1997 requested review from alecandido and felixhekhorn July 12, 2022 13:00

Improve functions' names consistency

3010f8c

felixhekhorn approved these changes Jul 12, 2022

View reviewed changes

alecandido approved these changes Jul 12, 2022

View reviewed changes

andreab1997 merged commit 9fea7f4 into main Jul 12, 2022

delete-merged-branch Bot deleted the sv_check branch July 12, 2022 15:22

Conversation

andreab1997 commented Jun 13, 2022

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

felixhekhorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andreab1997 commented Jun 17, 2022

Uh oh!

felixhekhorn commented Jun 17, 2022

Uh oh!

andreab1997 commented Jun 17, 2022

Uh oh!

felixhekhorn commented Jun 17, 2022

Uh oh!

Uh oh!

Uh oh!

felixhekhorn commented Jul 5, 2022

Uh oh!

andreab1997 commented Jul 5, 2022

Uh oh!

andreab1997 commented Jul 5, 2022

Uh oh!

felixhekhorn commented Jul 5, 2022

Uh oh!

andreab1997 commented Jul 9, 2022

Uh oh!

alecandido left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alecandido Jul 9, 2022

Choose a reason for hiding this comment

Uh oh!

felixhekhorn Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreab1997 Jul 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixhekhorn commented Jul 11, 2022

Uh oh!

felixhekhorn commented Jul 11, 2022

Uh oh!

andreab1997 commented Jul 12, 2022

Uh oh!

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

Uh oh!

Uh oh!

andreab1997 commented Jul 12, 2022

Uh oh!

andreab1997 commented Jul 12, 2022

Uh oh!

felixhekhorn commented Jul 12, 2022

Uh oh!

andreab1997 commented Jul 12, 2022

Uh oh!

andreab1997 commented Jul 12, 2022

Uh oh!

Reviewers

Assignees

codecov Bot commented Jun 13, 2022 •

edited

Loading

alecandido Jul 11, 2022 •

edited

Loading

alecandido commented Jul 12, 2022 •

edited

Loading