Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add runcards (and runner) for integrability observables #150

Merged
merged 6 commits into from
Aug 25, 2022

Conversation

scarlehoff
Copy link
Member

@scarlehoff scarlehoff commented Aug 18, 2022

I've only added the two we use in the evolution basis fit but it also works with the ones used in the flavour-basis fit.

I've already checked that the numbers are equal to those of the integrability fktables.

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2022

Codecov Report

Merging #150 (4c7238b) into master (54d4a55) will increase coverage by 0.66%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   29.53%   30.19%   +0.66%     
==========================================
  Files          23       24       +1     
  Lines        1141     1212      +71     
==========================================
+ Hits          337      366      +29     
- Misses        804      846      +42     
Flag Coverage Δ
unittests 30.19% <41.66%> (+0.66%) ⬆️

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

Impacted Files Coverage Δ
pinefarm/external/integrability.py 41.17% <41.17%> (ø)
pinefarm/info.py 64.00% <50.00%> (-4.19%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • please remember to install pre-commit (consider that sooner or later also here we will opt into pydocstyle)
  • even fine to merge for now
  • to generate the associated FK tables Fixing integrability opcards pineko#41 of course is a pre-requisite

pinecards/NNPDF_INTEG_XT3_40/integrability.yaml Outdated Show resolved Hide resolved
@scarlehoff
Copy link
Member Author

please remember to install pre-commit (consider that sooner or later also here we will opt into pydocstyle)

poetry install pre-commit ?

Moreover given that, changing Q0 is not completely out of scope

Actually, I guess it makes more sense if I take Q0 from the theory instead?

@felixhekhorn
Copy link
Contributor

poetry install pre-commit ?

actually pre-commit and poetry conflict with each other ;-) (since each of them wants to manage a python env ... ) - so pre-commit should be (in some sense) installed globally - see also https://pre-commit.com/ ; actually in such a ways that git can access it

Moreover given that, changing Q0 is not completely out of scope

Actually, I guess it makes more sense if I take Q0 from the theory instead?

if you want to enforce the two numbers are always the same, yes

@scarlehoff
Copy link
Member Author

scarlehoff commented Aug 18, 2022

This can be merged.

if you want to enforce the two numbers are always the same, yes

Yes I do. I've done that now.

@scarlehoff
Copy link
Member Author

That's weird. I did run pre-commit before the last commit.

@felixhekhorn
Copy link
Contributor

That's weird. I did run pre-commit before the last commit.

then this should not have happend ... mmm ... I was about to say "pre-commit also prevents stuff like this, which can be caused by different black version e.g. because it gets pinned"

@felixhekhorn
Copy link
Contributor

That's weird. I did run pre-commit before the last commit.

@scarlehoff stupid question: you did activate pre-commit in the repo? What happens if you run pre-commit run --all-files?

@scarlehoff
Copy link
Member Author

Nothing.

But I think it is because your previous pre-commit caused a conflict (I had not pulled) so I rolled back before pushing. Probably undid also the pre-commit. My bad.

@scarlehoff
Copy link
Member Author

scarlehoff commented Aug 18, 2022

I'll merge this then? Or do you want me to change the x to 1.0 now that the pineappl bugfix is done?

@felixhekhorn
Copy link
Contributor

either way ... @cschwan do we want to fix the affected datasets after closing NNPDF/pineappl#167? (as said also #132 are affected)

in any case people are starting to run fits (e.g. @giacomomagni ) and they need all datasets

@scarlehoff
Copy link
Member Author

Then I'll merge and i'll fix it at a later stage

@scarlehoff scarlehoff merged commit b7f3429 into master Aug 25, 2022
@cschwan
Copy link
Contributor

cschwan commented Aug 25, 2022

@felixhekhorn if something is wrong we should fix it!

@felixhekhorn
Copy link
Contributor

@felixhekhorn if something is wrong we should fix it!

there is not necessarily something wrong on putting the trivial x2 point something different then 1.0 (it is not used after all), however I'd suggest to standardize and put them to 1.0, so that's what we would need to "fix" - I think integrability are the only pinecards affected (all others are at 1.0 (and that's why they fail ...))

@felixhekhorn felixhekhorn deleted the integrability_fk branch August 25, 2022 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants