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

keeping MET sumPtUnclustered (backport of #28370) #29276

Merged
merged 3 commits into from Mar 30, 2020

Conversation

danbarto
Copy link
Contributor

PR description:

The sum of the unclustered energy is one of the inputs to MET significance, but is currently not stored in miniAOD, making recomputation of the significance and the MET covariance matrix difficult. With this PR, the calculated sum of the pTs of the unclustered particles is stored, and can be accessed with metSumPtUnclustered(), similar to metSignificance().

PR validation:

All tests have been run, and were passed.

if this PR is a backport please specify the original PR and why you need to backport that PR:

backport of #28370, needed for nanoAODv7

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 23, 2020

A new Pull Request was created by @danbarto (Daniel) for CMSSW_10_2_X.

It involves the following packages:

DataFormats/PatCandidates
PhysicsTools/PatAlgos
RecoMET/METAlgorithms
RecoMET/METProducers

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @emilbols, @peruzzim, @seemasharmafnal, @mmarionncern, @ahinzmann, @smoortga, @jdolen, @ferencek, @rovere, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @clelange, @riga, @JyothsnaKomaragiri, @mverzett, @cbernet, @gpetruc, @andrzejnovak this is something you requested to watch as well.
@davidlange6, @silviodonato, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@slava77
Copy link
Contributor

slava77 commented Mar 23, 2020

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 23, 2020

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/5334/console Started: 2020/03/23 21:28

@peruzzim
Copy link
Contributor

thank you really much @danbarto for providing this backport

@peruzzim peruzzim mentioned this pull request Mar 23, 2020
3 tasks
@cmsbuild
Copy link
Contributor

+1
Tested at: aab6c2e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6622b1/5334/summary.html
CMSSW: CMSSW_10_2_X_2020-03-23-1100
SCRAM_ARCH: slc6_amd64_gcc700

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6622b1/5334/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007491
  • DQMHistoTests: Total failures: 4
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007297
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@@ -250,6 +254,8 @@ namespace pat {

// MET significance
double metSig_;
// MET metSumPtUnclustered for MET Significance
double metSumPtUnclustered_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to change the name of this private class member wrt the one in the 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.

Sorry, my mistake. I'll fix that.

<class name="pat::MET" ClassVersion="15">
<version ClassVersion="15" checksum="428901429"/>
<class name="pat::MET" ClassVersion="16">
<version ClassVersion="16" checksum="2969659150"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones @slava77 (also for my own understanding): is it correct that the same ClassVersion (here "16") can get two different checksum's in different releases (here 10_2 vs 11_1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Once a checksum has been assigned a version number it should not be changed. The only exception is when it is deemed that version was never used for any useful jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • changes of existing versions in a release are not allowed
  • same versions in different releases with different checksums are not allowed

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @Dr15Jones and @slava77

I assume then that, once metSumPtUnclustered_ is renamed as sumPtUnclustered_. as in the master, the very same structure in terms of class members and methods will be there for this backport and the master. Therefore the same version "16" and checksum as the master can be also backported.

Copy link
Contributor

Choose a reason for hiding this comment

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

if for whatever reason we have to increment a version number here, we should make sure to add it to the master as well

Copy link
Contributor

Choose a reason for hiding this comment

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

if for whatever reason we have to increment a version number here, we should make sure to add it to the master as well

Once class members and methods are sinchronized as suggested above, the same class version can be considered for both releases.

The only difference in the code itself between the master and the 10_2 backport will come from the application of the clang format updates in the master, and not yet in 10_2.

That is not enough to trigger a version number change. (And even if so, then probably both

DataFormats/PatCandidates/interface/MET.h
DataFormats/PatCandidates/src/MET.cc

can be directly copied from the version in the master, and at that point they will be absolutely identical in the two CMSSW releases).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After fixing the inconsistencies in MET.h/MET.cc the version is now consistent with the master. I'll update the PR soon.

@@ -44,7 +44,8 @@ namespace metsig {
JME::JetResolution & resPtObj,
JME::JetResolution & resPhiObj,
JME::JetResolutionScaleFactor & resSFObj,
bool isRealData);
bool isRealData,
double& sumPtUnclustered);
Copy link
Contributor

Choose a reason for hiding this comment

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

fix indentation

@@ -54,7 +54,8 @@ metsig::METSignificance::getCovariance(const edm::View<reco::Jet>& jets,
JME::JetResolution& resPtObj,
JME::JetResolution& resPhiObj,
JME::JetResolutionScaleFactor& resSFObj,
bool isRealData) {
bool isRealData,
double& sumPtUnclustered) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fix indentation

@cmsbuild
Copy link
Contributor

Pull request #29276 was updated. @perrotta, @cmsbuild, @santocch, @slava77 can you please check and sign again.

@perrotta
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18f2ad/5374/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 214 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007475
  • DQMHistoTests: Total failures: 33
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007252
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 9.013 KiB( 30 files compared)
  • DQMHistoSizes: changed ( 1325.7 ): 9.013 KiB Physics/NanoAODDQM
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 26, 2020

The tests are being triggered in jenkins.
Tested with other pull request(s) #29294,cms-sw/cmsdist#5697
Test Parameters:

@cmsbuild
Copy link
Contributor

+1
Tested at: f08ddc4
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18f2ad/5398/summary.html
CMSSW: CMSSW_10_2_X_2020-03-26-1100
SCRAM_ARCH: slc6_amd64_gcc700

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18f2ad/5398/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • ROOTFileChecks: Some differences in event products or their sizes found
  • Reco comparison results: 214 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007475
  • DQMHistoTests: Total failures: 34
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007251
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 9.013 KiB( 30 files compared)
  • DQMHistoSizes: changed ( 1325.7 ): 9.013 KiB Physics/NanoAODDQM
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@perrotta
Copy link
Contributor

+1

  • "double sumPtUnclustered" added to the MET DataFormat
  • Jenkins tests pass and show no other differences than this additional double added to the dataformat; changes observable in the output of automatic tests originate from NanoAODv7 code updates [102X] #29294, which was run together in those tests
  • Since only one additional double is added to the pat::MET, while all other class members stay untouched, I think this can fulfil the requirements for a backport. The algorithm to read from it will need to be enabled with an era modifier.

@silviodonato
Copy link
Contributor

merge
@santocch

@cmsbuild cmsbuild merged commit d19efa1 into cms-sw:CMSSW_10_2_X Mar 30, 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 CMSSW_10_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_11_1_X is complete. This pull request will be automatically merged.

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

8 participants