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

PPS nanoAOD #31531

Merged
merged 30 commits into from
Dec 16, 2020
Merged

PPS nanoAOD #31531

merged 30 commits into from
Dec 16, 2020

Conversation

jwill24
Copy link
Contributor

@jwill24 jwill24 commented Sep 22, 2020

PR description:

Provides the relevant PPS information in the nanoAOD in the form of FlatTables. The added content includes:

  • Proton variables
  • Proton track variables
  • LHC database information

Updates regarding these additions can be found in the slides: https://indico.cern.ch/event/923848/contributions/3881642/attachments/2048614/3450447/jwilliams_nanoaod.pdf

PR validation:

These changes have been shown to function as expected when running over both data and MC -- adding the PPS tables when running with data and no additional changes when running with MC. The code has been used to process nanoAOD samples for the full Run II dataset (DoubleEG and EGamma streams). Slides of the analysis using these samples can be found here: https://indico.cern.ch/event/927936/contributions/3904334/attachments/2057549/3452032/pps_diphoton.pdf

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31531/18510

Code check has found code style and quality issues which could be resolved by applying following patch(s)

protonTable
+singleRPTable
+multiRPTable
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need some of these precomputed in the miniAOD ?
if yes, we do not have in the previous miniAOD so you need to set the empty sequence for those
check here
(run2_miniAOD_80XLegacy | run2_nanoAOD_94XMiniAODv1 | run2_nanoAOD_94XMiniAODv2 | run2_nanoAOD_94X2016 | run2_nanoAOD_102Xv1 | run2_nanoAOD_106Xv1).toReplaceWith(protonTables, cms.Sequence())

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "precomputed"? I naively thought that nanoAOD is build from miniAOD and the nano tables are built in this step.

Copy link
Contributor

@mariadalfonso mariadalfonso Sep 22, 2020

Choose a reason for hiding this comment

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

The current master branch should be able to run Nano also with input from previously (since 80X release) made miniAOD campaigns EOY, UL and the future UL-reMiniAOD.
So if the input collections are available since 80X release and have stable meaning, we are ok.
Not sure when the PPS entered miniAOD.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mariadalfonso , I understand now. PPS has consistent miniAOD only as of the Run2 UL re-reco. What what would you recommend for the era-modifier condition?

Copy link
Contributor

Choose a reason for hiding this comment

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

import the modifiers

from Configuration.Eras.Modifier_run2_miniAOD_80XLegacy_cff import run2_miniAOD_80XLegacy
from Configuration.Eras.Modifier_run2_nanoAOD_94X2016_cff import run2_nanoAOD_94X2016
from Configuration.Eras.Modifier_run2_nanoAOD_94XMiniAODv1_cff import run2_nanoAOD_94XMiniAODv1
from Configuration.Eras.Modifier_run2_nanoAOD_94XMiniAODv2_cff import run2_nanoAOD_94XMiniAODv2
from Configuration.Eras.Modifier_run2_nanoAOD_106Xv1_cff import run2_nanoAOD_106Xv1

just put an empty sequence when the PPS is not defined ;)

(run2_miniAOD_80XLegacy | run2_nanoAOD_94XMiniAODv1 | run2_nanoAOD_94XMiniAODv2 | run2_nanoAOD_94X2016 | run2_nanoAOD_102Xv1 | run2_nanoAOD_106Xv1).toReplaceWith(protonTables, cms.Sequence())

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-31531/18527

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jwill24 (Justin Williams) for master.

It involves the following packages:

PhysicsTools/NanoAOD

@gouskos, @cmsbuild, @fgolf, @mariadalfonso, @santocch, @peruzzim can you please review it and eventually sign? Thanks.
@gpetruc this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@mariadalfonso
Copy link
Contributor

@jwill24

can you confirm that it runs successfully the following:

runTheMatrix.py -l limited -i all --ibeos;
runTheMatrix.py -l 136.8522;

@silviodonato
Copy link
Contributor

@mariadalfonso @perrotta are there some pending comments from your side?
@jan-kaspar @jwill24 do you think the PR is ready? Is there something still missing?

@perrotta
Copy link
Contributor

@mariadalfonso @perrotta are there some pending comments from your side?
@jan-kaspar @jwill24 do you think the PR is ready? Is there something still missing?

This code is only affecting nanoAOD, and reco is only a secondary actor here.
I have added a couple of (additional) minor comments about the only file which ends up in the reco part: just in case there are other modification requested or planned (or maybe even if tests will need to be re-run), please take also them into account

@jan-kaspar
Copy link
Contributor

I don't have further comments. I agree with the suggestion by @perrotta above.

@mariadalfonso
Copy link
Contributor

Three new tables are included for PPS: singleRP, multiRP, PPSLocalTrack added in the event tree, only data are filled.
Information on beamEnergy, crossingAngle, betaStar added for the lumi tree.

Data size increase is about +6.8% for dataUL18 and +0.2% for data UL17.
Simulation is not updated.

Last presentation at XPOG https://indico.cern.ch/event/971101/#3-pps-mc-simulation-and-minina
clarified the long term strategy:

  • Tracks should not be added to the nano like we do not add tracks of interest of BPH ,btagging ....
  • LHC information will be likely removed from the tree and replaced by utilities as discussed a the PC workshop
  • simulation should follow the same strategy as of the other detectors i.e. enter other data tiers at earlier stage.
    The overall PPS strategy for nano should be completely reworked.

We consider this a temporary update to allow gain experience, as in nano we need to store only high level objects that can be used by non experts.

DQM plots attached below
singleRP
multiRP
PPSLocalTrack

@mariadalfonso
Copy link
Contributor

+xpog
#31531 (comment)

@perrotta
Copy link
Contributor

+1

  • For reco, a RecoPPS/ProtonReconstruction/plugins/PPSFilteredProtonProducer.cc has been added, only used in nanoAOD so far
  • (Homework on it: please replace with a range based loop the ones at L185 and L221, where the pr_idx counter can be replaced in the debug output respectively with n_protons_single_rp_all and n_protons_multi_rp_all)
  • Jenkins tests pass and show no differences in reco outputs wrt the baseline, as expected

@antoniovilela
Copy link
Contributor

Three new tables are included for PPS: singleRP, multiRP, PPSLocalTrack added in the event tree, only data are filled.
Information on beamEnergy, crossingAngle, betaStar added for the lumi tree.

Data size increase is about +6.8% for dataUL18 and +0.2% for data UL17.
Simulation is not updated.

Last presentation at XPOG https://indico.cern.ch/event/971101/#3-pps-mc-simulation-and-minina
clarified the long term strategy:

* Tracks should not be added to the nano like we do not add tracks of interest of BPH ,btagging ....

* LHC information will be likely removed from the tree and replaced by utilities as discussed a the PC workshop

Concerning this particular issue, please do let us know when such utilities are available.

We would like to stress that, especially the crossing angle but also beta* are needed variables for analyses.

We access them from the Event Setup. We should certainly check that we are saving them at the needed precision -- we have a well defined range of crossing angle and beta* values. So these will only take a small amount of bits per event. Any external utility should be able to correctly assign the beam parameters with a LS granularity.

PPS, AlCaDB and others spent considerable effort in putting in place the (online+offline) DB infrastructure for this information to be readily available for reconstruction and analysis. What level of size reduction are we talking about by removing these variables.

* simulation should follow the same strategy as of the other detectors i.e. enter other data tiers at earlier stage.
  The overall PPS strategy for nano should be completely reworked.

We consider this a temporary update to allow gain experience, as in nano we need to store only high level objects that can be used by non experts.

DQM plots attached below
singleRP
multiRP
PPSLocalTrack

@silviodonato
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 04f7b97 into cms-sw:master Dec 16, 2020
@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

@mariadalfonso
Copy link
Contributor

@jwill24 and @jan-kaspar
I expect you backport this in the 10_6_X so that we will have in the v9nano when reading the reMiniAOD

@jan-kaspar
Copy link
Contributor

Many thanks for pointing this out! We schedule this task - what would be the optimal time line?

@mariadalfonso
Copy link
Contributor

@jan-kaspar
we are not in a tight schedule, I hope we can close this and other PPS nano needs in January

@jan-kaspar
Copy link
Contributor

OK, noted, many thanks!

@jan-kaspar
Copy link
Contributor

@jwill24 and @jan-kaspar
I expect you backport this in the 10_6_X so that we will have in the v9nano when reading the reMiniAOD

Done in #32616.

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.

9 participants