Polish legacy jet datasets by comane · Pull Request #2056 · NNPDF/nnpdf

comane · 2024-04-17T15:59:56Z

The scope of this PR is to (try to) polish the new implementation of the legacy jet datasets, namely, CMS_2JET_7TEV, CMS_1JET_8TEV, ATLAS_1JET_8TEV_R06, ATLAS_2JET_7TEV_R06.

What the PR does:

Adds a filter_utils.utils.py module. This module should collect the functions that are generalisable and could, in principle, be used for all datasets.
Adds a filter_utils.legacy_jets_utils.py module that collects all of the utils functions for the filter files. Polishing these is going to be much harder given the nature of the raw data.

Radonirinaunimi

Thanks @comane for this! I will have a detailed look soon, but when skimming through I think there are at least two main issues with the CMS_1JET_8TEV_PTY dataset:

the variable in the kinematic has changed from p_T to p_T2 while in the metadata it's still kept as p_T
the file uncertainties_bugged.yaml no longer exists but it is still referenced in the metadata

RoyStegeman · 2024-04-24T17:34:03Z

This was not done in this PR, but I changed it since I don't think we should start getting in the habit of suppressing warnings if there is another option

comane · 2024-04-24T17:40:14Z

This was not done in this PR, but I changed it since I don't think we should start getting in the habit of suppressing warnings if there is another option

Thanks @RoyStegeman, just to be sure I understand: removing the read "r" from with open avoids the pandas warnings?

RoyStegeman · 2024-04-24T17:42:33Z

No, removing "r" is just because that's the default option anyway.

The warnings are fixed around line 900 in legacy_jets_utils.py.

A similar error, with similar solution, that I didn't fix still exists for ATLAS_2JET_7TEV_R06. I didn't fix it now because that filter.py is broken anyway.

RoyStegeman · 2024-04-24T17:45:01Z

Since filter files will now share utilities, and we may need to change/fix those shared utilities at some point - do you think it's feasible to run all filters in the CI to test if the output remains unchanged?

RoyStegeman · 2024-04-24T18:00:56Z

nnpdf/validphys2/src/validphys/process_options.py

Line 27 in eed00c7

p_T2 = "p_T2" # This one is wrong, should be pT2

@scarlehoff what should be done in this PR?

scarlehoff · 2024-04-24T20:30:04Z

@scarlehoff what should be done in this PR?

Change it to pT2 I'd say.

RoyStegeman · 2024-05-06T13:26:38Z

@comane I see you addressed the CI fails (as pointed out by @Radonirinaunimi), but could you rename p_T2 to pT2?

comane · 2024-05-07T16:11:20Z

@comane I see you addressed the CI fails (as pointed out by @Radonirinaunimi), but could you rename p_T2 to pT2?

Sure, I am not sure why though?
Who wrote that comment there and why is p_T2 wrong? The tests seem to be checking for p_T2 at the moment, not for pT2

So maybe I can simply remove that comment?

RoyStegeman · 2024-05-07T16:24:07Z

I assume @scarlehoff wrote it. Either way I agree with it for consistency with pT and pT_t

RoyStegeman · 2024-05-15T10:02:02Z

        label: $|y|$
        units: ''
-      pT:
+      pT2:


Why did was this change made (I see the corresponding change was also done in kinematics.yaml)?

I am not completely sure, but I think this was discussed with @scarlehoff at some point.
The reason is probably that tanishq started using pT2 instead of pT, hence just to uniform to that.

I set it back to pT. I think this makes more sense as is the kin var for the process JET

RoyStegeman · 2024-05-15T10:34:19Z

+    return {"min": min, "mid": mid, "max": max}
+
+
+# ==================================================================== CMS_2JET_7TEV ====================================================================#


If these are only used for a single dataset, why are they not inside the folder of the corresponding dataset?

Yes, this is how it was before, where I had a filter_utils.py module in each of the datasets.

I guess that the idea is that in the dataset folders we only have a filter.py that produces the dataset, all of the utils instead go in nnpdf_data/filter_utils.
Otherwise we would have some utils in nnpdf_data/filter_utils and other within the dataset folder, which would be more confusing.

scarlehoff

Please add a __init__.py file to filter_utils as well.

In all honesty, I'm not entirely sure I see the point of having a legacy_jet_utils.py function where the functions inside are actually specific for each dataset.

However, I'm ok with concentrating all that stuff into filter_utils so that it can be easily skipped by the installation with the right rules in pyproject.toml.

… legacy one

…d filter_utils.py to filter_utils.legacy_jets_utils.py

I only address the 1JET case (though solution for 2JET is similar), because the 2JET filter.py is broken anyway

Co-authored-by: Roy Stegeman <roystegeman@live.nl>

Radonirinaunimi

This LGTM now! Thanks @comane.

@scarlehoff, are you fine with merging this now? Such that I can continue with #2100 and #2099.

scarlehoff · 2024-06-07T07:25:56Z

Yes. I think it should be ok.

comane requested review from Radonirinaunimi, RoyStegeman and scarlehoff April 17, 2024 15:59

Radonirinaunimi reviewed Apr 18, 2024

View reviewed changes

scarlehoff mentioned this pull request Apr 24, 2024

Polarised Jet commondata implementation #2035

Merged

RoyStegeman force-pushed the polish_legacy_jet_datasets branch from 346e661 to f66e959 Compare April 24, 2024 17:33

RoyStegeman force-pushed the polish_legacy_jet_datasets branch from 7138ebe to 517b046 Compare April 24, 2024 18:37

comane force-pushed the polish_legacy_jet_datasets branch from 1276509 to 8cf8687 Compare May 7, 2024 16:43

RoyStegeman reviewed May 15, 2024

View reviewed changes

scarlehoff mentioned this pull request May 20, 2024

To Do for 4.0.10 #1854

Closed

33 tasks

scarlehoff reviewed May 22, 2024

View reviewed changes

Comment thread nnpdf_data/nnpdf_data/new_commondata/ATLAS_2JET_7TEV_R06/uncertainties.yaml Outdated

Comment thread nnpdf_data/nnpdf_data/filter_utils/legacy_jets_utils.py Outdated

Radonirinaunimi mentioned this pull request Jun 5, 2024

Fix new commondata filters #2100

Merged

scarlehoff added the data toolchain label Jun 5, 2024

comane and others added 8 commits June 6, 2024 14:28

added general utils and legacy_jets_utils

b2f342a

CMS_1JET_8TEV files changed based on change in metadata names

112797f

moved filter_utils of CMS_1JET_8TEV into legacy jet utils

363b62d

removed filter_bugged and uncertainties_bugged as we already have the…

503bdab

… legacy one

ATLAS_1JET_8TEV, checked that filter.py runs, polished filter.py move…

e8e01d4

…d filter_utils.py to filter_utils.legacy_jets_utils.py

ATLAS_1JET_8TEV, checked that filter.py runs, polished filter.py move…

777c3d6

…d filter_utils.py to filter_utils.legacy_jets_utils.py

corrected kinematical variable from sqrt_s to sqrts

bbf5a7a

Don't surpress warnings and instead address them

da13195

I only address the 1JET case (though solution for 2JET is similar), because the 2JET filter.py is broken anyway

RoyStegeman and others added 15 commits June 6, 2024 14:28

black+isort

234d880

tiny cleanup

793a5f7

pT to p_T2 in metadata file

b216b55

polished ATLAS_2JET_7TEV

f4babdd

changed cut for CMS_1JET_8TEV from pT to p_T2

39c776a

no p_T2 no more

b10bd47

fill df with columns of np.nan

01b9b28

Update validphys2/src/validphys/cuts/filters.yaml

a05b336

Co-authored-by: Roy Stegeman <roystegeman@live.nl>

Update validphys2/src/validphys/cuts/lockfiles/31_filters.lock.yaml

aaae251

Co-authored-by: Roy Stegeman <roystegeman@live.nl>

changed docstring

9a44d65

added init file and float prettifier to filter utils

04cfd57

float accuracy of ATLAS_1JET_8TEV

5bcaefc

8 decimals for CMS_2JET_7TEV

5cd09ea

8 decimals for ATLAS_2JET_7TEV_R06

ec0f0ce

8 decimals for CMS_1JET_8TEV uncertainties and data

88fd513

comane force-pushed the polish_legacy_jet_datasets branch from f7dd15a to 88fd513 Compare June 6, 2024 13:56

comane added 2 commits June 6, 2024 16:41

pT2 to pT for JET type of process

9d3f74d

pT2 -> pT in filters.yaml

425cfa0

Radonirinaunimi approved these changes Jun 7, 2024

View reviewed changes

Merge branch 'master' into polish_legacy_jet_datasets

80b1161

comane merged commit 4472155 into master Jun 10, 2024

comane deleted the polish_legacy_jet_datasets branch June 10, 2024 09:28

		return {"min": min, "mid": mid, "max": max}


		# ==================================================================== CMS_2JET_7TEV ====================================================================#

Conversation

comane commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Radonirinaunimi left a comment

Choose a reason for hiding this comment

Uh oh!

RoyStegeman commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comane commented Apr 24, 2024

Uh oh!

RoyStegeman commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoyStegeman commented Apr 24, 2024

Uh oh!

RoyStegeman commented Apr 24, 2024

Uh oh!

scarlehoff commented Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RoyStegeman commented May 6, 2024

Uh oh!

comane commented May 7, 2024

Uh oh!

RoyStegeman commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RoyStegeman May 15, 2024

Choose a reason for hiding this comment

Uh oh!

comane May 18, 2024

Choose a reason for hiding this comment

Uh oh!

comane Jun 6, 2024

Choose a reason for hiding this comment

Uh oh!

RoyStegeman May 15, 2024

Choose a reason for hiding this comment

Uh oh!

comane May 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scarlehoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Radonirinaunimi left a comment

Choose a reason for hiding this comment

Uh oh!

scarlehoff commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

comane commented Apr 17, 2024 •

edited

Loading

RoyStegeman commented Apr 24, 2024 •

edited

Loading

RoyStegeman commented Apr 24, 2024 •

edited

Loading

scarlehoff commented Apr 24, 2024 •

edited

Loading

RoyStegeman commented May 7, 2024 •

edited

Loading