Skip to content

Update to new commondata names - n3fit - #2031

Merged
RoyStegeman merged 16 commits into
masterfrom
update_ds_names
Apr 10, 2024
Merged

Update to new commondata names - n3fit#2031
RoyStegeman merged 16 commits into
masterfrom
update_ds_names

Conversation

@RoyStegeman

@RoyStegeman RoyStegeman commented Mar 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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 Bot commented Mar 29, 2024

Copy link
Copy Markdown

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 Bot commented Mar 29, 2024

Copy link
Copy Markdown

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

@scarlehoff scarlehoff left a comment

Copy link
Copy Markdown
Member

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.

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

@scarlehoff scarlehoff Apr 8, 2024

Copy link
Copy Markdown
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 ?)

@andreab1997 andreab1997 Apr 8, 2024

Copy link
Copy Markdown
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.

@scarlehoff scarlehoff Apr 8, 2024

Copy link
Copy Markdown
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.

Comment thread n3fit/src/n3fit/tests/test_evolven3fit.py Outdated
@RoyStegeman RoyStegeman changed the title Update to new commondata names in docs and examples Update to new commondata names - n3fit Apr 8, 2024
@RoyStegeman

RoyStegeman commented Apr 8, 2024

Copy link
Copy Markdown
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

RoyStegeman commented Apr 8, 2024

Copy link
Copy Markdown
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
@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

@scarlehoff scarlehoff scarlehoff approved these changes
+1 more reviewer
@andreab1997 andreab1997 andreab1997 left review comments
Reviewers whose approvals may not affect merge requirements

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