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

adding new keys to theory template #51

Merged
merged 2 commits into from
Oct 20, 2023
Merged

adding new keys to theory template #51

merged 2 commits into from
Oct 20, 2023

Conversation

giacomomagni
Copy link
Collaborator

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

@alecandido
Copy link
Member

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
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
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
Contributor

please fix pre-commit and then this should be fine

@giacomomagni giacomomagni merged commit da22c52 into main Oct 20, 2023
6 checks passed
@delete-merged-branch delete-merged-branch bot deleted the update_theory branch October 20, 2023 13:57
@alecandido
Copy link
Member

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