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

Provide an unique truth for all theory cards #2028

Merged
merged 65 commits into from
May 2, 2024
Merged

Conversation

andreab1997
Copy link
Contributor

@andreab1997 andreab1997 commented Mar 28, 2024

Here we address #2019.

  • Create a dataclass collecting all the possible parameters that can appear in a theory card, removing the ones that are not used anymore
  • Add utilities function to read a theory which can be also used by pineko
  • Provide a way to read and/or search in all theories

cc @scarlehoff

@andreab1997 andreab1997 linked an issue Mar 28, 2024 that may be closed by this pull request
@andreab1997 andreab1997 changed the title First minimal example Provide an unique truth for all theory cards Mar 28, 2024
@andreab1997
Copy link
Contributor Author

At the moment if you see # it means that I am not sure if the parameter is still used or not. I am adding now comments and questions for all the problematic ones

nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
@andreab1997 andreab1997 self-assigned this Mar 28, 2024
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
@scarlehoff
Copy link
Member

Thanks @andreab1997, let's have next week as the target to merge this

With respect to the missing items:

Add utilities function to read a theory which can be also used by pineko

I think this can be decoupled

Provide a way to read and/or search in all theories

I'll add this to #2037 (but I think it can also be decoupled)

@andreab1997
Copy link
Contributor Author

Thanks @andreab1997, let's have next week as the target to merge this

With respect to the missing items:

Add utilities function to read a theory which can be also used by pineko

I think this can be decoupled

Provide a way to read and/or search in all theories

I'll add this to #2037 (but I think it can also be decoupled)

Ok agreed. Do you want me to do something before merging it?

Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

Some comments.

nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/theory.py Outdated Show resolved Hide resolved
@RoyStegeman
Copy link
Member

Since I believe this is almost final, don't forget to update (the theory_params.csv file in) the docs

@RoyStegeman
Copy link
Member

One thing I just realised is that by having defaults we make it even harder to search within the theorydb based on theoryparameters. Of course I'll concede that since they are the defaults it's probably not particularly useful to use it as a filter

@scarlehoff
Copy link
Member

Having defaults doesn't mean we should not fill in a value. The main goal of the defaults is for the old runcards not to fail and allow us to remove some keys going forward.

So, even if some keys are added with a default we can decide that new theories must have a value defined (while leaving old theories alone). For now in #2062 Q0 is mandatory again.

@RoyStegeman
Copy link
Member

RoyStegeman commented Apr 25, 2024

But all old theories have an explicit entry for e.g. alphas (and all other values that are given defaults in this PR), so why would those not be made mandatory now. There is no need to give defaults for older theories

@scarlehoff
Copy link
Member

But all old theories have an explicit entry for e.g. alphas (and all other values that are given defaults in this PR), so why would those not be made mandatory now. There is no need to give defaults for older theories.

The idea is that some of the values in the old theories were either redundant or not needed. But maybe we were overeager and there's some that have been moved to optional that shouldn't be.

(if so, please mention which ones are those and we can make them mandatory again!)

@RoyStegeman
Copy link
Member

RoyStegeman commented Apr 25, 2024

wrt to this PR I would not add defaults for MaxNfPdf, Q0, Qref, alphaqed, HQ

@scarlehoff scarlehoff added the run-fit-bot Starts fit bot from a PR. label Apr 30, 2024
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@scarlehoff
Copy link
Member

If nobody is explicitly against this, I'll merge this PR on Thursday.

@RoyStegeman I'd appreciate a second look at the 41000000.yml file since I've made a few changes.

@RoyStegeman
Copy link
Member

@RoyStegeman I'd appreciate a second look at the 41000000.yml file since I've made a few changes

As you know I'm happy with explicitly defining parameters in the runcard instead of relying on the defaults, so for me this is not a huge problem, but IterEv and HQ are set to the default values so they can also be removed.

@scarlehoff
Copy link
Member

I also prefer having explicit values even if defaults are defined.

@RoyStegeman
Copy link
Member

Good, other than that 41000000.yml seems fine

@scarlehoff scarlehoff merged commit 400e5c1 into master May 2, 2024
6 checks passed
@scarlehoff scarlehoff deleted the single_truth branch May 2, 2024 08:29
@scarlehoff scarlehoff removed the run-fit-bot Starts fit bot from a PR. label May 2, 2024
@scarlehoff scarlehoff mentioned this pull request May 20, 2024
33 tasks
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.

Create a single source of truth for all theories
7 participants