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.

Sign up for GitHub

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

Jump to bottom

Update to new commondata names - n3fit #2031

Merged
merged 16 commits into from
Apr 10, 2024
Merged

Update to new commondata names - n3fit #2031

merged 16 commits into from
Apr 10, 2024

Conversation

RoyStegeman
Copy link
Member

@RoyStegeman RoyStegeman commented Mar 29, 2024 •
edited
Loading

Closes some of #1969

This PR makes changes to the dataset names in the following

  • Remove reproduce 4.0
  • Update runcards in n3fit
  • Update docstrings in n3fit
  • Update tests in n3fit

@RoyStegeman RoyStegeman added the run-fit-bot Starts fit bot from a PR. label Mar 29, 2024
@github-actions GitHub Actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@RoyStegeman RoyStegeman added run-fit-bot Starts fit bot from a PR. and removed run-fit-bot Starts fit bot from a PR. labels Mar 29, 2024
@github-actions GitHub Actions
Copy link

Greetings from your nice fit 🤖 !
I have good news for you, I just finished my tasks:

Check the report carefully, and please buy me a ☕ , or better, a GPU 😉!

@RoyStegeman RoyStegeman force-pushed the update_ds_names branch 3 times, most recently from 2f9da88 to d752805 Compare April 3, 2024 15:25
@RoyStegeman RoyStegeman changed the base branch from master to remove_buildmaster April 3, 2024 15:26
Base automatically changed from remove_buildmaster to master April 3, 2024 22:14
scarlehoff
scarlehoff reviewed Apr 8, 2024
View reviewed changes
Copy link
Member

@scarlehoff scarlehoff left a comment

Choose a reason for hiding this comment

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

Thank you very much for this. I guess there's still some to go, but as far as I'm concerned we can merge it when you wish (no need to have updated all of it before merging!)

RE the conda profiles for reproduction. Maybe those we want to keep in the repo, since reading them with conda should take care of having the code in the state decided by the .yaml file.

n3fit/runcards/examples/nnpdf40-like.yml
Comment on lines +99 to +100
sampling:
separate_multiplicative: false
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering whether we should remove this key by now or set it to true going forward (@andreab1997 ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to be backward compatible with NNPDF40 we cannot remove the key. Otherwise, if we no longer care about backward compatibility, given that we are approaching 4.1, I agree we should remove the key and just set it to False. Note that the NNPDF40 behaviour is recovered with true not with false.

Copy link
Member

Choose a reason for hiding this comment

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

For 1-1 compatibility there is the conda environments provided. I think from 4.0.10 onwards we are happy with slowly moving away from 4.0 compatibility.

n3fit/src/n3fit/tests/test_evolven3fit.py Outdated Show resolved Hide resolved
@RoyStegeman RoyStegeman changed the title Update to new commondata names in docs and examples Update to new commondata names - n3fit Apr 8, 2024
@RoyStegeman
Copy link
Member Author

RE the conda profiles for reproduction. Maybe those we want to keep in the repo, since reading them with conda should take care of having the code in the state decided by the .yaml file.

Yes that's true, I removed them because I don't believe they have ever been used and for some of them the corresponding nnpdf version has not been uploaded to the server since we were uploading upon merge to master only and not upon tagging. Thinking about it again though, it's true that the dependencies might come in handy at some point, so perhaps we can keep them around a bit longer.

@RoyStegeman
Copy link
Member Author

guess there's still some to go, but as far as I'm concerned we can merge it when you wish (no need to have updated all of it before merging!)

Yes maybe that's better, I'm not sure exactly when I'll have enough time to finish everything so we may as well merge what we have and I'll continue in a a new branch

If you're happy please approve

@RoyStegeman RoyStegeman changed the base branch from master to fix_keras_3 April 10, 2024 10:11
Base automatically changed from fix_keras_3 to master April 10, 2024 10:42
@RoyStegeman RoyStegeman merged commit 61006aa into master Apr 10, 2024
6 checks passed
@RoyStegeman RoyStegeman deleted the update_ds_names branch April 10, 2024 14:37
@RoyStegeman RoyStegeman mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreab1997 andreab1997 andreab1997 left review comments

@scarlehoff scarlehoff scarlehoff approved these changes

Assignees
No one assigned
Labels
run-fit-bot Starts fit bot from a PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RoyStegeman @scarlehoff @andreab1997