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

[12_5_X] [L1T] Backport of: Phase-2, include missing collections in outputs #39451

Merged
merged 8 commits into from Oct 12, 2022

Conversation

cecilecaillol
Copy link
Contributor

@cecilecaillol cecilecaillol commented Sep 20, 2022

PR description:

This PR addresses the missing Tk object collections in the CMSSW_12_5_0_pre5 RelVals. It includes fixes were needed to avoid errors after adding in the objects to the EventContent. One error that came up in the FEVTDEBUGHLToutput_step was: The class "l1t::TkJetWord" is compiled and for its data member "tkJetWord_" we do not have a dictionary for the collection "bitset<70>". Because of this, we will not be able to read or write this data member.
classes_def.xml was adjusted to avoid the error. Local PR: cms-l1t-offline#1048

This PR also adds missing collections for Phase 2 tau-jet-met to the event content. Specifically the 9x9 Sums for histogrammed jets, the NN Puppi taus, and the HPS taus. Local PR: cms-l1t-offline#1047

We need this PR in 12_5 for our Phase-2 L1 MC production

Backport of #39401 and #39499

@cecilecaillol
Copy link
Contributor Author

backport of #39401

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 20, 2022

A new Pull Request was created by @cecilecaillol for CMSSW_12_5_X.

It involves the following packages:

  • DataFormats/L1Trigger (l1)
  • L1Trigger/Configuration (l1)
  • L1Trigger/L1CaloTrigger (upgrade, l1)

@rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please review it and eventually sign? Thanks.
@kreczko, @rovere, @eyigitba, @Martin-Grunewald, @missirol, @dinyar, @thomreis, @trtomei, @beaucero this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06bf02/27665/summary.html
COMMIT: eadfae7
CMSSW: CMSSW_12_5_X_2022-09-19-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39451/27665/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3699454
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3699430
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@cecilecaillol
Copy link
Contributor Author

+l1

BenjaminRS and others added 6 commits October 3, 2022 10:35
(cherry picked from commit e6d49f5)
(cherry picked from commit 8ec852a)
(cherry picked from commit 3e7a998)
(cherry picked from commit d1362be)
(cherry picked from commit 57a365d)
(cherry picked from commit 9e4475e)
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2022

Pull request #39451 was updated. @rekovic, @epalencia, @cmsbuild, @AdrianoDee, @srimanob, @cecilecaillol can you please check and sign again.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 3, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-06bf02/27928/summary.html
COMMIT: 3aeb57b
CMSSW: CMSSW_12_5_X_2022-10-02-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39451/27928/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3699454
  • DQMHistoTests: Total failures: 8
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3699424
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@cecilecaillol
Copy link
Contributor Author

+l1

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

@cecilecaillol @BenjaminRS I assume all these collections are needed, and therefore the master PR had just been signed. Nonetheless, in this backport PR could you please add an estimate of the increase of the event size, possibly already in the PR description?

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

This PR backports both #39401 and #39499

@srimanob
Copy link
Contributor

srimanob commented Oct 5, 2022

+Upgrade

Backport PR. Waiting for size estimation for additional collections.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2022

This pull request is fully signed and it will be integrated in one of the next CMSSW_12_5_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_6_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@srimanob
Copy link
Contributor

srimanob commented Oct 7, 2022

Note to @perrotta @rappoccio
With this PR together with #39412, we should be good for the CMSSW_12_5 release for L1T production. Thanks.

@@ -220,7 +220,7 @@ void HPSPFTauProducer::fillDescriptions(edm::ConfigurationDescriptions& descript
desc.add<double>("minSeedJetPt", 30.0);
desc.add<double>("maxChargedRelIso", 1.0);
desc.add<double>("minSeedChargedPFCandPt", 5.0);
desc.add<edm::InputTag>("srcL1PFCands", edm::InputTag("l1ctLayer1", "PF"));
desc.add<edm::InputTag>("srcL1PFCands", edm::InputTag("l1tLayer1", "PF"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug, or the bug is in the master instead?

Copy link
Contributor

@srimanob srimanob Oct 11, 2022

Choose a reason for hiding this comment

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

It seems this line will be overwritten by
https://github.com/cms-sw/cmssw/blob/master/L1Trigger/Phase2L1Taus/python/HPSPFTauProducerPF_cfi.py#L5
srcL1PFCands = "l1tLayer1:PF",

However, let's @cms-sw/l1-l2 confirms.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this line will be overwritten by https://github.com/cms-sw/cmssw/blob/master/L1Trigger/Phase2L1Taus/python/HPSPFTauProducerPF_cfi.py#L5 srcL1PFCands = "l1tLayer1:PF",

Sure, it is overwritten.
But it changes the default.
Can we have the correct default in both master and backports?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Let's spend a little time to clean up things.

In addition, I see few places in master that still use l1ctLayer1 which I assume them to change to l1tLayer1
https://github.com/cms-sw/cmssw/search?q=l1ctLayer1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is correct. I will make a bug-fix PR to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR to master with the fixes: #39704

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes: #39705

@perrotta
Copy link
Contributor

perrotta commented Oct 7, 2022

@cecilecaillol besides #39451 (review) (which must be clarified) this is a verbatim backport of #39401 and #39499. Once that gets fixed, this PR can be merged in 12_5.

Before merging, please also provide an estimate of the increase in the event size, as it was already requested long time ago. A gross estimate is enough, and it should be rather simple to derive it starting from the data samples you have at hand.

@rappoccio
Copy link
Contributor

@cecilecaillol ping on this?

@srimanob
Copy link
Contributor

srimanob commented Oct 11, 2022

Regarding the size, measure with TTbar PU200
/eos/cms/store/group/offcomp_upgrade-sw/srimanob/Phase2Fall22L1T/outputs/step3-PU200.root

The collections (listed belowed) add 12776.2 Bytes/Event (Average Compressed Size)

l1tEtSums_l1tPhase1JetProducer9x9_UncalibratedPhase1L1TJetFromPfCandidatesMET_HLT. 830.2 637.6
l1tEtSums_l1tPhase1JetSumsProducer9x9_Sums_HLT. 732.2 539.6
l1tEtSums_l1tTrackerEmuEtMiss_L1TrackerEmuEtMiss_HLT. 639 563.6
l1tEtSums_l1tTrackerEmuHTMissExtended_L1TrackerEmuHTMissExtended_HLT. 683.8 607.6
l1tEtSums_l1tTrackerEmuHTMiss_L1TrackerEmuHTMiss_HLT. 639 563.6
l1tHPSPFTaus_l1tHPSPFTauProducerPF__HLT. 11314.2 1740.6
l1tHPSPFTaus_l1tHPSPFTauProducerPuppi__HLT. 6303 1260.4
l1tTkEtMisss_l1tTrackerEtMiss_l1tTrackerEtMiss_HLT. 699.8 570.8
l1tTkHTMisss_l1tTrackerHTMiss_L1TrackerHTMiss_HLT. 689 566.4
l1tTkJets_l1tTrackFastJets_L1TrackFastJets_HLT. 2963.2 869.2
l1tTkJets_l1tTrackJetsExtended_L1TrackJetsExtended_HLT. 5361.8 1199.6
l1tTkJets_l1tTrackJets_L1TrackJets_HLT. 2330.4 753.2
l1tTkJetWords_l1tTrackJetsEmulation_L1TrackJets_HLT. 1870.2 691.2
l1tTkJetWords_l1tTrackJetsExtendedEmulation_L1TrackJetsExtended_HLT. 2516.6 786.4
recoCaloJets_l1tPhase1JetCalibrator9x9_Phase1L1TJetFromPfCandidates_HLT. 1817.2 704.8
recoCaloJets_l1tPhase1JetProducer9x9_UncalibratedPhase1L1TJetFromPfCandidates_HLT. 1845.2 721.6

These collections are not available in CMSSW_12_6_0_pre2 relvals. I use relvals to x-check if the above collections are added on top as expected.

@perrotta
Copy link
Contributor

+1

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.

None yet

6 participants