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

Reimplement ATLAS Z0 7 TeV Low mass #2171

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

comane
Copy link
Member

@comane comane commented Oct 11, 2024

This pull request introduces a filtering module for the ATLAS Z0 7 TeV low mass dataset.

Benchmark with master

(master) https://vp.nnpdf.science/6PBYe_IRTgiWJXpprEGH6Q==

(this branch) https://vp.nnpdf.science/wemXLoM2Sn-87lfEBreTCA==

Compatibility Checks

Covariance matrix check:

In [4]: from validphys.api import API
   ...: import numpy as np
   ...: 
   ...: inp1 = {"dataset_input": {"dataset": f"ATLAS_Z0_7TEV_LOMASS_M"}, "theoryid": 40_000_000, "use_cuts": "internal", "t0pdfset": "NNPDF40_nnlo_as_01
   ...: 180", "use_t0": True}
   ...: inp2 = {"dataset_input": {"dataset": f"ATLAS_Z0_7TEV_LOMASS_M", "variant": "legacy"}, "theoryid": 40_000_000, "use_cuts": "internal", "t0pdfset"
   ...: : "NNPDF40_nnlo_as_01180", "use_t0": True}
   ...: 
   ...: covmat1 = API.covmat_from_systematics(**inp1)
   ...: covmat2 = API.covmat_from_systematics(**inp2)
   ...: 
   ...: t0_covmat1 = API.t0_covmat_from_systematics(**inp1)
   ...: t0_covmat2 = API.t0_covmat_from_systematics(**inp2)
   ...: 
   ...: print(np.all(np.isclose(covmat1, covmat2)))
   ...: print(np.all(np.isclose(t0_covmat1, t0_covmat2)))
True
True

@comane
Copy link
Member Author

comane commented Oct 11, 2024

Hi @scarlehoff @enocera , tests are failing I think because stat unc are forced to be ADD and UNCORR.
However, for this dataset the stat unc are provided as MULT by hepdata.

I now set the stat unc to ADD as it makes no difference at the level of the covmat.

@comane comane requested a review from ecole41 October 11, 2024 14:23
@scarlehoff
Copy link
Member

stat unc are forced to be ADD and UNCORR.

This is correct. I wonder whether having them mult will break something, like the replica generation or closure tests, where the systematics are perhaps being taken explicitly separated.

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.

Hi @comane thanks for this. Could you also set the kinematic_override to be the identity and use one of the process types in the library of process option

This most likely will break something and you might need to change the choice of kinematic variables (from pt to pt2 or smt) and it will also define what to do for the rest of the DY data (the same will be necessary in your datasets @jacoterh @achiefa).

The reason for this is that we want to get rid of these "kinematic overrides" and instead use the right variables directly in the datasets.

(I think you don't need to implement new x-Q mappings or processes because we already had some DY datasets in the data for the pheno project)

@comane comane force-pushed the reimplement_ATLAS_Z0_7TEV_LOMASS branch from c785a42 to 699177f Compare October 21, 2024 12:53
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.

Thanks.

The things missing here are:

  • Change the kinematic variable from k_i to their actual names
  • The kinematics_override should be removed or set to the identity
  • The process_type must be change to one of the processes in
    accepted_variables=(_Vars.y, _Vars.eta, _Vars.m_W2, _Vars.m_Z2, _Vars.sqrts),
  • Version comment, bump the version and remove the "Port of old commondata"

@comane comane force-pushed the reimplement_ATLAS_Z0_7TEV_LOMASS branch from 699177f to 5eb61fb Compare December 7, 2024 14:37
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need this file in the history, so it'd be better if you squash your commits and force push I think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The filter doesn't seem to use this file, it uses the .csv instead. Is there any reason to keep both?

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.

3 participants