Skip to content

adding new keys to theory template#51

Merged
giacomomagni merged 2 commits into
mainfrom
update_theory
Oct 20, 2023
Merged

adding new keys to theory template#51
giacomomagni merged 2 commits into
mainfrom
update_theory

Conversation

@giacomomagni
Copy link
Copy Markdown
Collaborator

Adding two missing keys to the theory template: FONLLparts and n3lo_cf_variation

@alecandido
Copy link
Copy Markdown
Collaborator

Even fine, but isn't this definitely breaking the database?

If you don't use the database (and benchmarking infrastructure), there should be no need to use banana.
I don't remember if we're good enough to ignore extra keys. But I wouldn't expect.

@giacomomagni
Copy link
Copy Markdown
Collaborator Author

giacomomagni commented Oct 20, 2023

I'm adding them to make tests in this PR passing:

https://github.com/NNPDF/yadism/actions/runs/6578472057/job/17872298906?pr=215

I believe this to be needed because I have explicitly modified:

https://github.com/NNPDF/yadism/blob/21f09fa81e33e2762315d21394969dcf262a98ce/src/yadism/input/domains.yaml

If my reasoning is correct, the other option would be to drop the changes in domains.yaml as is currently done in
NNPDF/yadism#195

If you agree, just tell me which option you prefer. (or maybe if you have better one...)

@felixhekhorn
Copy link
Copy Markdown
Contributor

I'm adding them to make tests in this PR passing:

If you don't use the database (and benchmarking infrastructure), there should be no need to use banana.

just as I need 0.6.9 to make PR xyz pass, so should be fine

Even fine, but isn't this definitely breaking the database?

it will break - just as many of the v0.6.x tags did (if we we serious about this we should be now at at least v0.12)

I don't remember if we're good enough to ignore extra keys. But I wouldn't expect.

we were not - and the problem is we need it (instead of being too much)

@felixhekhorn
Copy link
Copy Markdown
Contributor

please fix pre-commit and then this should be fine

@giacomomagni giacomomagni merged commit da22c52 into main Oct 20, 2023
@delete-merged-branch delete-merged-branch Bot deleted the update_theory branch October 20, 2023 13:57
@alecandido
Copy link
Copy Markdown
Collaborator

If my reasoning is correct, the other option would be to drop the changes in domains.yaml as is currently done in
NNPDF/yadism#195

At the moment I've been quite busy with other commitments, but I will soon get rid of domains.yaml (as promised long ago).

@RoyStegeman
Copy link
Copy Markdown
Member

Just to note that FONLLParts is an optional argument

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