-
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 ATLAS Z0 7TEV 46FB Dataset #2237
Conversation
Hi @ecole41 this is not ready for rewview yet right? I see that the variables are still called k1/k2/k_i etc for instance. |
No, this is not ready yet. I will keep working on it |
Ok! No problem! Is any of the PR finished? There are now many and I got a bit lost. So that I can review (and hopefully merge) the ones that are |
…0_7TEV_46FB_CC_AND_CF
This branch should be complete, but is failing the checks. I have merged from the master so am not sure why this is happening |
Indeed, it's really odd - I don't see what's wrong with your metadata. The yaml file cannot be parsed for some reason. I'm looking into it. |
variants: | ||
legacy: | ||
data_uncertainties: | ||
- uncertainties_legacy_CC-Y.yaml | ||
data_central: data_legacy_CC-Y.yaml | ||
data_central: |
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.
I noticed that if you remove data_central from variants: legacy
the tests pass again
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.
Ok, I will just check if changing the formatting of this helps
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 tests pass now, excellent!
nnpdf_data/nnpdf_data/commondata/ATLAS_Z0_7TEV_46FB/metadata.yaml
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/ATLAS_Z0_7TEV_46FB/metadata.yaml
Outdated
Show resolved
Hide resolved
nnpdf_data/nnpdf_data/commondata/ATLAS_Z0_7TEV_46FB/metadata.yaml
Outdated
Show resolved
Hide resolved
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.
Looks like this is good to go after the minor comments I left are implemented. Thanks!
This can be merged, right? (I see it is approved, but just checking!) |
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.
Just the comment on the labels, for both datasets (like in #2223 I see the check was done with the old names though, here's a link with the check with the new names: https://vp.nnpdf.science/eS7CgNDEQMmflyqxTbBmdQ==
label: k1 | ||
abs_eta: | ||
description: Absolute dilepton rapidity | ||
label: abs_eta |
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.
Eventually this will be the label to be used in plots, so it would be better to use |\eta|
or m_{ll}^2
(below) etc.
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.
Ok, I have changed the format in the metadata. Is this format correct?
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.
Yes, that works. Since it is rapidity, like in the other PR it might make sense to change the labels to "y" already.
Also, for sqrts, I think you are missing the \
in sqrt
but these are minor points.
Please have a look at the conflict and then this can be merged I believe.
label: k1 | ||
abs_eta: | ||
description: Absolute dilepton rapidity | ||
label: "|y|" |
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.
@scarlehoff Just wanted to check if this is now the correct labelling.
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.
I think in the one below (m_ll) you need to also put $
) for matplotlib to render it correctly. Not sure about the |
. But other than that, yes, looks good.
In the xlabel
though you might want to remove (in both here and the other observable) the \eta
part so that it is only the y.
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.
Ok, I have just changed this
After updating the compatibility test to use the new dataset name, I get that the matrices are compatible within 0.1% except the covariance matrix for the CF set which is compatible within 1%, so there are some changes in the new implementation :
Here are the updated old vs new reports: |
Thanks. If you solve the conflict I think this can be merged. |
This pull request is merging the two CC and CF datasets into one filter.py script.
Functions haves been added to produce the data central, kinematic and uncertainties yaml files for both of these datasets.
Old vs New Data
CC Dataset: https://vp.nnpdf.science/SpiOHitcS-2WggfpKcLW3Q==/
CF Dataset: https://vp.nnpdf.science/D3121rJ6RWG3IgyqViXJMw==/
Compatibility Check: