-
Notifications
You must be signed in to change notification settings - Fork 7
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
Reimplement DY #2248
base: master
Are you sure you want to change the base?
Reimplement DY #2248
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @peterkrack! Happy new year and thanks for this!
Here are some preliminary comments:
- Could you please fix the metedata regarding the arXiv, inSpire, and HepData urls? These will be crucial when moving into Remove HepData raw tables from the repository #2228
- Could you make sure that the filters run properly. Currently, this is not the case as these filters contain some calls to non-existing functions.
- Add the following to the filters to properly format the floats:
nnpdf/nnpdf_data/nnpdf_data/commondata/ATHENA_NC_45GEV_EP/filter.py
Lines 12 to 14 in 3f59024
from nnpdf_data.filter_utils.utils import prettify_float yaml.add_representer(float, prettify_float) - Use pre-commit hooks https://pre-commit.com/ to make sure that all the files are properly formatted before each commit.
Could you please take of these first before I can move further into the details of the implementation?
nnpdf_data/nnpdf_data/commondata/DYE605_Z0_38P8GEV_DW/filter.py
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/DYE605_Z0_38P8GEV_DW/filter.py
Outdated
Show resolved
Hide resolved
Hi @peterkrack, please let me know if there is anything I can help with in the meantime. |
422f1d1
to
567ec45
Compare
I was wondering if there are some updates regarding the pending issues above and whether some helps are needed? |
For DYE605_Z0_38P8GEV_DW is suspect it is a bug in the old buildmaster. The other two I have to figure out what exactly is causing the discrepancy between legacy and reimplementation. |
I can also have a look in details, but would you be able to take care of #2248 (review) above? |
567ec45
to
fb1b0a7
Compare
bf2ff28
to
44c117e
Compare
Concerning the extra uncertainty: Then in the old buildmaster nnpdf/buildmaster/filters/FTDY.cc Line 151 in f6c49ae
then later on the loop runs from irep=0 to irep=999; one uncertainty too much nnpdf/buildmaster/filters/FTDY.cc Line 206 in f6c49ae
|
Dear @peterkrack @Radonirinaunimi, let me try to clarify the uncertainties for the DY E605 data set.
I see two ways of proceeding forward.
|
I have a preference for option 2. |
Thanks for your reply @enocera! I agree with all of your points - and the conclusions. As you see in the report, this differences is negligible so I tend to lean towards the 2nd option (on top of the reason you said). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kinematics_override
needs to be set to the identity. The result_transform
we can live with for the time being.
Probably the process type needs to be changed as well, we shoudn't have a process type per dataset...
This is probably very minor, but how would you call the process types then? And are you happy with how the variants are called? |
The variables are ok. For the process type... use DYP, otherwise you will need to dive into validphys to change a few things... although FTDY would be better imho but 🤷♂️ |
Yep, this I know. But I was wondering if you want something specific after the
Accounting for this, I went for |
yes, that sounds ok, as long as the first three letters are DYP it will go through vp ok. It needs to be added to process options though. |
When modifying the process options, I went for the easiest solution which is to simply add the variable |
reimplemented: | ||
data_uncertainties: | ||
- uncertainties_reimplemented_PXSEC.yaml | ||
data_central: data_reimplemented_PXSEC.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final comments. This should not be reimplemented
, this should just be the normal data_uncertainties
, not a variant.
Also, if we have data_reimplemented
and it is the same as the old one, the old one should be removed. And I think for one of the 866 it needs to be kept because there were small differences in the data (in the rawdata)
Same for all the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For one out of the four datasets, the new implementation has a slightly different central values (numerical fluctuations due to rawdata source), so not sure if we want to keep reimplemented
for that one (?). But for the rest, I'll do asap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we want to keep the old data under legacy
for that dataset, and the new implementation be the default for all (so not keep reimplemented
for any)
Overall, this is close to completion except for
DYE605_Z0_38P8GEV_DW_PXSEC
in which the legacy implementation contains one extra-source of systematic uncertainties (cc @enocera).These being said, none of the remaining differences are really visible at the data vs theory comparisons report.