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

Observable card generators for pDIS #232

Merged
merged 29 commits into from
Jul 12, 2024
Merged

Observable card generators for pDIS #232

merged 29 commits into from
Jul 12, 2024

Conversation

Radonirinaunimi
Copy link
Member

@Radonirinaunimi Radonirinaunimi commented Oct 12, 2023

@toonhasenack, please implement and update here the implementation of the observable cards generator.

PS: Recall that to generate the observable cards you need to be in extra/data and run python -m machinery generate E155/*.

The full list of datasets can be found in NNPDF/nnpdf#1816

@Radonirinaunimi Radonirinaunimi marked this pull request as draft October 12, 2023 15:19
@felixhekhorn felixhekhorn added the physics physics features label Oct 12, 2023
@alecandido alecandido changed the title [WIP] Observable card generators for pDIS Observable card generators for pDIS Oct 12, 2023
@alecandido
Copy link
Member

Draft mode is enough for WIP declaration :)

@Radonirinaunimi
Copy link
Member Author

Draft mode is enough for WIP declaration :)

That was the OCD part of me 😅

@felixhekhorn
Copy link
Contributor

@toonhasenack please activate pre-commit on your computer! which would have told you that you committed something broken 🙃 (Unfortunately pydocstyle is currently broken - but that's the only one you're allowed to ignore)

@giacomomagni
Copy link
Collaborator

giacomomagni commented Feb 15, 2024

@Radonirinaunimi, since we know the kinematics of the pDIS datasets is correct,
can we merge this?
I've just left a minor comment.

@giacomomagni giacomomagni marked this pull request as ready for review February 15, 2024 16:51
extras/data/COMPASS/metadata.yaml Outdated Show resolved Hide resolved
extras/data/COMPASS/metadata.yaml Outdated Show resolved Hide resolved
extras/data/E142/metadata.yaml Outdated Show resolved Hide resolved
extras/data/E143/metadata.yaml Outdated Show resolved Hide resolved
extras/data/E154/metadata.yaml Outdated Show resolved Hide resolved
extras/data/E155/metadata.yaml Outdated Show resolved Hide resolved
extras/data/EMC/metadata.yaml Outdated Show resolved Hide resolved
extras/data/HERMES/metadata.yaml Outdated Show resolved Hide resolved
extras/data/SMC/metadata.yaml Outdated Show resolved Hide resolved
@Radonirinaunimi
Copy link
Member Author

@toonhasenack, you undid the changes done by the hooks when you force-pushed. Could you please make sure to at least pull before pushing in order to avoid these.

@felixhekhorn
Copy link
Contributor

@toonhasenack, you undid the changes done by the hooks when you force-pushed. Could you please make sure to at least pull before pushing in order to avoid these.

or simply do as I say 🙃

extras/data/machinery/generate/__init__.py Outdated Show resolved Hide resolved
extras/data/COMPASS15/metadata.yaml Outdated Show resolved Hide resolved
extras/data/HERMES97/metadata.yaml Outdated Show resolved Hide resolved
@giacomomagni
Copy link
Collaborator

giacomomagni commented Mar 28, 2024

@felixhekhorn, @toonhasenack, @Radonirinaunimi are you happy to merge?

@felixhekhorn
Copy link
Contributor

when you believe you're ready, please open an accompanying PR in pinecards (copying the output here) and check that they work well also there; please don't merge here until then

please do this first - I'll test a few of them there

@giacomomagni
Copy link
Collaborator

please do this first - I'll test a few of them there

Note that some pinecards have already been merged on master in this PR,
so and I think @toonhasenack is using the updated ones.

@felixhekhorn
Copy link
Contributor

please do this first - I'll test a few of them there

Note that some pinecards have already been merged on master in this PR, so and I think @toonhasenack is using the updated ones.

Well, then I'd say it is even more relevant to fix them also there - I'm looking to the "Files changed"-tab and I don't like what I see
grafik

@giacomomagni
Copy link
Collaborator

Well, then I'd say it is even more relevant to fix them also there - I'm looking to the "Files changed"-tab and I don't like what I see !

sure before of this dcb59c7 all the metadata were even read at all!!

@Radonirinaunimi
Copy link
Member Author

What is still missing in this PR (so maybe I could take care)?

@giacomomagni
Copy link
Collaborator

giacomomagni commented Apr 2, 2024

Green light from my side.

@Radonirinaunimi
Copy link
Member Author

Green light from my side.

How about you @felixhekhorn?

@felixhekhorn
Copy link
Contributor

when you believe you're ready, please open an accompanying PR in pinecards (copying the output here) and check that they work well also there; please don't merge here until then

please do this first - I'll test a few of them there

I have the vague feeling I'm repeating myself ... 🙃

@giacomomagni giacomomagni added datatoolchain observable card generator and removed physics physics features labels May 1, 2024
@giacomomagni
Copy link
Collaborator

Here is the associated PR in pinecards: NNPDF/pinecards#181

@giacomomagni
Copy link
Collaborator

I think we should be able to merge. Do you have further comments @felixhekhorn ?

@giacomomagni giacomomagni merged commit 8210ba9 into master Jul 12, 2024
5 checks passed
@giacomomagni giacomomagni deleted the generators-pDIS branch July 12, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datatoolchain observable card generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants