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

Update fonll docstr and json loading #163

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

scarlehoff
Copy link
Member

While updating this line

Update by reading the yamldb of the original theory.

I tried to run it just because and noticed that not all fktables have the theories saved as valid json. In particular some of the HERA fktables in 701XX sometimes have single quotes instead of double so I've added a "bypass" for that as well (and a few boolean values are also wrong). Not sure why it happened.

@scarlehoff scarlehoff requested a review from giacomomagni March 25, 2024 09:48
@scarlehoff scarlehoff changed the base branch from main to use_nnpdf_data March 25, 2024 09:48
@giacomomagni
Copy link
Contributor

Thanks for spotting it. Which are the keys with the wrong syntax?
Maybe we should correct this also in pinefarm or yadism

@giacomomagni giacomomagni added the bug Something isn't working label Mar 25, 2024
@scarlehoff
Copy link
Member Author

scarlehoff commented Mar 25, 2024

Which are the keys with the wrong syntax?

There're a few grids where the theory key uses ' instead of " (which is invalid json) for all keys.

For the True instead of true, I don't remember.

@felixhekhorn
Copy link
Contributor

Thanks for spotting it. Which are the keys with the wrong syntax? Maybe we should correct this also in pinefarm or yadism

I was sure that pinefarm writes the theory as meta data, but I can't find it https://github.com/NNPDF/pinefarm/blob/39abf325d3ab8ec227f7c296935e55f966184434/src/pinefarm/external/interface.py#L120 what am I missing? (and it should be fixed in pinefarm)

@scarlehoff
Copy link
Member Author

I was sure that pinefarm writes the theory as meta data

The theory is not even used by all runners so it might actually better that this is not the case yet... (or that only the ones that use it actually write it).

@scarlehoff scarlehoff merged commit 43862a7 into use_nnpdf_data Mar 25, 2024
5 checks passed
@scarlehoff scarlehoff deleted the quickfix_fonll branch March 25, 2024 14:13
@scarlehoff
Copy link
Member Author

I was sure that pinefarm writes the theory as meta data,

@felixhekhorn I was curious and checked that yadism actually writes this using json.dumps so the error was, in theory, impossible.

Indeed, the error is generated by pineko but I don't understand how and why it happens only for some datasets and not others.
If you are curious, HERACOMB_SIGMARED_B in theory 701 has this problem.

I'll open an issue about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants