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 to new commondata names - validphys #2074

Merged
merged 13 commits into from
Jul 31, 2024
Merged

Conversation

RoyStegeman
Copy link
Member

After dealing with the dataset names in n3fit in #2031, this PR continues to deal with the vp side.

closes #1969

@scarlehoff
Copy link
Member

What's missing here? (I think the errors are just the fact that the plots have changed as well, right?)

@RoyStegeman
Copy link
Member Author

Yes, but so far I fixed far from all mentions of the old names so there is still a lot to do I'm afraid

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.

@peterkrack for some of tests to pass, you need to rebase this on top of master. While for the figures (in test_plots), you need to re-generate them.

@peterkrack
Copy link
Contributor

I tried to replace ATLASTTBARTOT with ATLAS_TTBAR_7TEV_TOT_X-SEC because I did not find any corresponding dataset among the new_commondata set names. test_plots.py fails with the new dataset. I am getting NotImplementedError: The process type INC for ATLAS_TTBAR_7TEV_TOT_X-SEC is not implemented. If I remove the ATLAS dataset from test_plots.py the test passes, at least when running it on stoomboot.

@Radonirinaunimi
Copy link
Member

I am getting NotImplementedError: The process type INC for ATLAS_TTBAR_7TEV_TOT_X-SEC is not implemented. If I remove the ATLAS dataset from test_plots.py the test passes, at least when running it on stoomboot.

This one can be solved by simply rebasing, and indeed it is now.

I tried to replace ATLASTTBARTOT with ATLAS_TTBAR_7TEV_TOT_X-SEC because I did not find any corresponding dataset among the new_commondata set names. test_plots.py fails with the new dataset.

It shouldn't be a problem which datasets you use to check the plotting. I you use ATLAS_TTBAR_7TEV_TOT_X-SEC, as indeed there is no correspondence to ATLASTTBARTOT in dataset_names.yml, you just need to generate the plots and that'll become the new reference.

@scarlehoff
Copy link
Member

I've rebased on top of master and updated the regression tests.

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.

I've tested the tests but haven't gone over every change of name in detail, so I'm just trusting they are sensible.

Just the one comment about the author in the tests (I've also opened the following issue #2130 related to that)

@@ -1,7 +1,7 @@
meta:
title: PDF flavours plot example
author: Rosalyn Pearson
keywords: [example]
author: Rosalyn L. Pearson
Copy link
Member

@scarlehoff scarlehoff Jul 18, 2024

Choose a reason for hiding this comment

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

I would honestly change the authors in the example from any to Lazy Person or The Creator or some other funny thing like that. Mainly so that if people uploads a report without changing these fields we know that we don't know who uploaded it instead of thinking it was Rosalyn :__

@scarlehoff scarlehoff merged commit b0ca369 into master Jul 31, 2024
6 checks passed
@scarlehoff scarlehoff deleted the update_vp_dsnames branch July 31, 2024 14:15
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.

Update to new commondata names in docs, examples and tests
5 participants