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

Polish EIC data files #2039

Merged
merged 10 commits into from
Apr 11, 2024
Merged

Polish EIC data files #2039

merged 10 commits into from
Apr 11, 2024

Conversation

comane
Copy link
Member

@comane comane commented Apr 8, 2024

This PR aims at polishing the ATHENA data files.
Things that are done:

  • moved all xlsx files into a rawdata folder
  • added openpyxl to poetry extras. This is needed by pd.read_excel
  • created an athena_utils.py file so as to minimise repeated code.

@comane
Copy link
Member Author

comane commented Apr 8, 2024

@scarlehoff @Radonirinaunimi @RoyStegeman I think we should probably have a nnpdf_data/filter_utils folder that contains all filter utils modules for datasets. What do you think

@RoyStegeman
Copy link
Member

Thanks, I'll have a closer look later, but I agree that, if it makes sense to have multiple filter utils with e.g. some corresponding to only a single experiment, it would be good to have a filter_utils folder. That's exactly the sort of thing that's needed

@Radonirinaunimi
Copy link
Member

Thanks so much for this @comane!

@scarlehoff @Radonirinaunimi @RoyStegeman I think we should probably have a nnpdf_data/filter_utils folder that contains all filter utils modules for datasets. What do you think

Yes, something along that line should definitely be adopted, and as a matter of fact I was planning to discuss with you (later this week) about the main utils that were/are required in the datasets you implemented in order to draft something.

In this respect, as far as this PR is concerned, you could already move athena_ultis.py in a folder nnpdf_data/filter_utils.

nnpdf_data/nnpdf_data/athena_utils.py Outdated Show resolved Hide resolved
nnpdf_data/nnpdf_data/athena_utils.py Outdated Show resolved Hide resolved
@Radonirinaunimi Radonirinaunimi added the Polarised Polarised PDF fits label Apr 9, 2024
@Radonirinaunimi Radonirinaunimi changed the title Polish ATHENA data files Polish EIC data files Apr 9, 2024
@Radonirinaunimi
Copy link
Member

@comane, @RoyStegeman, given that these Athena share quite a lot of the functions with the other EIC projections, I also fixed them here. I therefore renamed the module from athena_utils $\longrightarrow$ eic_utils.

@RoyStegeman
Copy link
Member

Please rebase so the tests will pass

Copy link
Member

@RoyStegeman RoyStegeman left a comment

Choose a reason for hiding this comment

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

Thanks, seems fine to me

nnpdf_data/nnpdf_data/filter_utils/eic_utils.py Outdated Show resolved Hide resolved
@Radonirinaunimi Radonirinaunimi force-pushed the 240408_polish_filterfiles_1 branch 2 times, most recently from 1dc9baf to ec7c090 Compare April 11, 2024 10:04
Copy link
Member

@Radonirinaunimi Radonirinaunimi left a comment

Choose a reason for hiding this comment

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

@comane I think this is done now, please merge if you are fine with this.

@comane
Copy link
Member Author

comane commented Apr 11, 2024

thanks @Radonirinaunimi and @RoyStegeman for reviewing!

@comane comane force-pushed the 240408_polish_filterfiles_1 branch from 3b6cf1d to ee8921f Compare April 11, 2024 16:54
@RoyStegeman
Copy link
Member

You only force-pushed your local changes. I still have the commits from the review locally, would you like me to push those?

@comane
Copy link
Member Author

comane commented Apr 11, 2024

Yes please, I might have done a mess.
I had rebased on master but perhaps I didn't pull from the branch :(

@RoyStegeman RoyStegeman merged commit 7fb6b69 into master Apr 11, 2024
6 checks passed
@RoyStegeman RoyStegeman deleted the 240408_polish_filterfiles_1 branch April 11, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants