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

HGCal ID variables #32519

Merged

Conversation

SohamBhattacharya
Copy link
Contributor

@SohamBhattacharya SohamBhattacharya commented Dec 17, 2020

PR description:

This PR adds the following tools/producers for HGCal egamma:

  1. HGCalShowerShapeHelper (calculates shower shape variables like shower widths)
  2. HGCalClusterTools (calculates isolation quantities using HGCal layer-clusters)
  3. HGCalID variable producer (ID variable producer required for egamma HLT)

Backport PR for 11_1_x: #32517
Backport PR for 11_2_x: #32520

Test config:

cmsDriver.py hltTest --python_filename egHLTPhaseIITest_cfg.py --eventcontent AODSIM --customise SLHCUpgradeSimulations/Configuration/aging.customise_aging_1000,Configuration/DataProcessing/Utils.addMonitoring --step RAW2DIGI,RECO --datatier AODSIM  --conditions auto:phase2_realistic_T15 --geometry Extended2026D49 --nThreads 4 --filein /store/mc/Phase2HLTTDRSummer20ReRECOMiniAOD/DoubleElectron_FlatPt-1To100/GEN-SIM-DIGI-RAW-MINIAOD/PU200_111X_mcRun4_realistic_T15_v1-v2/280000/1B09CC5D-FAA3-5442-BE7B-96302835E860.root --era Phase2C9 --no_exec --mc --customise_commands "process.load('RecoEgamma.EgammaHLTProducers.HLTEgPhaseIITestSequence_cff');process.HLT_EgPhaseIITest_v1 = cms.Path(process.HLTEgPhaseIITestSequence);process.schedule.append(process.HLT_EgPhaseIITest_v1);process.AODSIMoutput.outputCommands.extend(['keep recoEcalCandidates*_*_*_*','keep recoRecoEcalCandidatesToValuefloatAssociation_*_*_*'])" -n 12 --processName=RECO2

Shower-shape variables studies:
https://indico.cern.ch/event/879938/contributions/4128109/attachments/2153463/3631734/presentation_20201201_EGammaGeneral.pdf#page=31

Isolation variable (using ClusterTools) studies:
https://indico.cern.ch/event/879938/contributions/4124438/attachments/2153449/3631661/CMSweek_Dec2020_EGM_Swagata.pdf#page=4

@cmsbuild cmsbuild changed the base branch from CMSSW_11_3_X to master December 17, 2020 11:01
@cmsbuild
Copy link
Contributor

@SohamBhattacharya, CMSSW_11_3_X branch is closed for direct updates. cms-bot is going to move this PR to master branch.
In future, please use cmssw master branch to submit your changes.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32519/20462

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @SohamBhattacharya for master.

It involves the following packages:

RecoEgamma/EgammaHLTProducers
RecoEgamma/EgammaTools

@perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please review it and eventually sign? Thanks.
@Sam-Harper, @sobhatta, @jainshilpi, @Fedespring, @lgray, @calderona, @HuguesBrun, @afiqaize, @varuns23, @cericeci, @ram1123 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

Comment on lines 1 to 26


#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"

#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateIsolation.h"
#include "DataFormats/EgammaReco/interface/SuperCluster.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"

#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/CaloRecHit/interface/CaloClusterFwd.h"

#include "DataFormats/EgammaReco/interface/SuperCluster.h"
#include "DataFormats/EgammaReco/interface/SuperClusterFwd.h"

#include "DataFormats/RecoCandidate/interface/RecoEcalCandidate.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h"

#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"

#include "RecoEgamma/EgammaTools/interface/HGCalShowerShapeHelper.h"
#include "RecoEgamma/EgammaTools/interface/HGCalClusterTools.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion:

Suggested change
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateIsolation.h"
#include "DataFormats/EgammaReco/interface/SuperCluster.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/CaloRecHit/interface/CaloClusterFwd.h"
#include "DataFormats/EgammaReco/interface/SuperCluster.h"
#include "DataFormats/EgammaReco/interface/SuperClusterFwd.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidate.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"
#include "RecoEgamma/EgammaTools/interface/HGCalShowerShapeHelper.h"
#include "RecoEgamma/EgammaTools/interface/HGCalClusterTools.h"
#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/CaloRecHit/interface/CaloClusterFwd.h"
#include "DataFormats/EgammaReco/interface/SuperCluster.h"
#include "DataFormats/EgammaReco/interface/SuperClusterFwd.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidate.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateFwd.h"
#include "DataFormats/RecoCandidate/interface/RecoEcalCandidateIsolation.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/ParameterSet/interface/ConfigurationDescriptions.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/ParameterSet/interface/ParameterSetDescription.h"
#include "RecoEgamma/EgammaTools/interface/HGCalClusterTools.h"
#include "RecoEgamma/EgammaTools/interface/HGCalShowerShapeHelper.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

yes agreed, it was sloppy of me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, done.

static void fillDescriptions(edm::ConfigurationDescriptions& descriptions);
void produce(edm::Event&, const edm::EventSetup&) override;

class PCAAssocMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

The way this class is used below should be fine - but the interfaces it presents exposes makes it a bit error prone.

Returning a std::unique_ptr<...>& has (at least ?) two problems:

  • whether ownership is taken from the object depends on what the reference is bound to (a reference, a value, nothing);
  • if bound to a reference, I think the behaviour relies on the object itself still being alive when the unique_ptr& is used; if it gets destroyed before the usage, one gets a dangling pointer
  • since one could end up with multiple references to the same unique_ptr, any could change or destroy it wihout the others knowing it

If the intended behaviour is that of shared ownership, it would be better to hold and return a std::sharet_ptr.
If the intended behaviour is to transfer ownership in a well-defined place (which I think is the case here), it would be better if

  • initMap() returns void (the return value is not used below)
  • operator() is renames to something like getMap() and returns std::unique_ptr<...> by value, not by reference

Copy link
Contributor

Choose a reason for hiding this comment

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

you're completely correct of course, will adjust

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32519/20463

  • This PR adds an extra 28KB to repository

@cmsbuild
Copy link
Contributor

Pull request #32519 was updated. @perrotta, @Martin-Grunewald, @slava77, @cmsbuild, @fwyzard, @jpata can you please check and sign again.

SohamBhattacharya added a commit to SohamBhattacharya/cmssw that referenced this pull request Dec 17, 2020
Comment on lines 4 to 46
// system include files
#include <memory>

// user include files
#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/EgammaCandidates/interface/GsfElectron.h"
#include "DataFormats/FWLite/interface/ESHandle.h"
#include "DataFormats/ForwardDetId/interface/HGCEEDetId.h"
#include "DataFormats/ForwardDetId/interface/HGCalDetId.h"
#include "DataFormats/GsfTrackReco/interface/GsfTrack.h"
#include "DataFormats/HGCRecHit/interface/HGCRecHit.h"
#include "DataFormats/Math/interface/LorentzVector.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHitFraction.h"
#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/StreamID.h"
#include "Geometry/CaloTopology/interface/HGCalTopology.h"
#include "Geometry/Records/interface/IdealGeometryRecord.h"
#include "RecoLocalCalo/HGCalRecAlgos/interface/RecHitTools.h"
#include "RecoParticleFlow/PFClusterProducer/interface/InitialClusteringStepBase.h"

#include <CLHEP/Vector/LorentzVector.h>

#include <algorithm>
#include <cstdlib>
#include <iostream>
#include <map>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

#include <TMatrixD.h>
#include <TVectorD.h>

#include <Math/Point3D.h>
#include <Math/Point3Dfwd.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

a suggestion:

Suggested change
// system include files
#include <memory>
// user include files
#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/EgammaCandidates/interface/GsfElectron.h"
#include "DataFormats/FWLite/interface/ESHandle.h"
#include "DataFormats/ForwardDetId/interface/HGCEEDetId.h"
#include "DataFormats/ForwardDetId/interface/HGCalDetId.h"
#include "DataFormats/GsfTrackReco/interface/GsfTrack.h"
#include "DataFormats/HGCRecHit/interface/HGCRecHit.h"
#include "DataFormats/Math/interface/LorentzVector.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHitFraction.h"
#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/StreamID.h"
#include "Geometry/CaloTopology/interface/HGCalTopology.h"
#include "Geometry/Records/interface/IdealGeometryRecord.h"
#include "RecoLocalCalo/HGCalRecAlgos/interface/RecHitTools.h"
#include "RecoParticleFlow/PFClusterProducer/interface/InitialClusteringStepBase.h"
#include <CLHEP/Vector/LorentzVector.h>
#include <algorithm>
#include <cstdlib>
#include <iostream>
#include <map>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
#include <TMatrixD.h>
#include <TVectorD.h>
#include <Math/Point3D.h>
#include <Math/Point3Dfwd.h>
// system include files
#include <algorithm>
#include <cstdlib>
#include <iostream>
#include <map>
#include <memory>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
// external include files
#include <CLHEP/Vector/LorentzVector.h>
#include <Math/Point3D.h>
#include <Math/Point3Dfwd.h>
#include <TMatrixD.h>
#include <TVectorD.h>
// CMSSW include files
#include "DataFormats/CaloRecHit/interface/CaloCluster.h"
#include "DataFormats/EgammaCandidates/interface/GsfElectron.h"
#include "DataFormats/FWLite/interface/ESHandle.h"
#include "DataFormats/ForwardDetId/interface/HGCEEDetId.h"
#include "DataFormats/ForwardDetId/interface/HGCalDetId.h"
#include "DataFormats/GsfTrackReco/interface/GsfTrack.h"
#include "DataFormats/HGCRecHit/interface/HGCRecHit.h"
#include "DataFormats/Math/interface/LorentzVector.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHit.h"
#include "DataFormats/ParticleFlowReco/interface/PFRecHitFraction.h"
#include "DataFormats/TrackReco/interface/Track.h"
#include "DataFormats/TrackReco/interface/TrackFwd.h"
#include "FWCore/Framework/interface/Event.h"
#include "FWCore/Framework/interface/Frameworkfwd.h"
#include "FWCore/Framework/interface/MakerMacros.h"
#include "FWCore/Framework/interface/stream/EDProducer.h"
#include "FWCore/ParameterSet/interface/ParameterSet.h"
#include "FWCore/Utilities/interface/StreamID.h"
#include "Geometry/CaloTopology/interface/HGCalTopology.h"
#include "Geometry/Records/interface/IdealGeometryRecord.h"
#include "RecoLocalCalo/HGCalRecAlgos/interface/RecHitTools.h"
#include "RecoParticleFlow/PFClusterProducer/interface/InitialClusteringStepBase.h"

@Sam-Harper
Copy link
Contributor

I have two concerns on the reco side with these helpers that cannot be included in the tests today:

  • a modification to these helpers (e.g. some esConsumes migration or the like) may introduce issues that are not visible to us in the tests. This could potentially be mitigated by requiring an explicit TSG signature for these files that are primarly used by TSG.
  • a POG/DPG developer may start to use these functions, not realizing that they have not been tested outside the TSG setup. If a workflow is provided by the developer at that moment, could be less of a concern.

While I do not disagree with your concerns I do feel this is a new or newly enforced policy (new could be a few years as HLT development hasnt been large since 2018) whose practical implications need to be considered. And I think this can be only done in a scheduled "face to face" meeting which we should have either next week or the week afterwards.

@Sam-Harper
Copy link
Contributor

So just to conclude here. Are we agreed that this PR and similar PRs can move forward given they have a sufficient example config now or do we still have to resolve this issue?

And that we agree to discuss the general policy and decide a way forward for the future in the next week or so.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b6ac5/11975/summary.html
CMSSW: CMSSW_11_3_X_2021-01-05-1100/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716967
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2716944
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@jpata
Copy link
Contributor

jpata commented Jan 6, 2021

So just to conclude here. Are we agreed that this PR and similar PRs can move forward given they have a sufficient example config now or do we still have to resolve this issue?

Given the one-off test config you contributed here, and the above discussion, I think this PR can move forward (will sign in a separate post).

To summarize, my position is that given test workflows cannot be provided currently, we need to be clear (as is done in #32594 by including L1, HLT etc in the names for common classes) that these committed classes are only for TSG use, and we will need to contact you for explicit validations should modifications to these classes be required. Overall, we need to work towards having more of the shared codebase testable automatically and regularly by all groups in a common environment, as reco cannot meaningfully sign code that we cannot run and do output diffs on. We can work towards a solution over the coming weeks, as it's clear that reco validation policy can evolve over time, given inputs from various groups.

@jpata
Copy link
Contributor

jpata commented Jan 6, 2021

+reconstruction

  • adds HGCAL ID variable producers and related helper classes for PhaseII HLT
  • no differences in tests (none are expected, as HLT code cannot currently run in any workflow or automatic test per the above discussion)
  • detailed validation was done by the TSG and is reported in the description (reco simply confirmed by hand that the mock HLT config dump including these modules runs using HGCal ID variables #32519 (comment))

@Sam-Harper
Copy link
Contributor

Thank you. However having L1 or HLT in the name is not sufficient. Identifying which is HLT or RECO or common code is difficult. Also what about code paths in a common class which only the HLT calls?

Note #32594 can also be used in the RECO. Its L1Trk isolation which can be used offline in reco (although not sure why you would want to but it is perfectly valid to use). So all the complains here equally apply to that PR. Only the HLT producers are likely HLT specific, the algos are intentionally shared as much as possible.

I am glad that we have agreed to meet soon to determine a solution to this, to be clear incase there are any missunderstanding, the HLT does wish (and to my knowledge does if not we should address this asap) to have a test of all code the HLT uses in jenkins and relvals via a periodic dump of the HLT. Its just there is a lag between the code going to the release and the tests becoming available due to the nature of the HLT and this is doubly so for the phase-II HLT. Shall we fix a time next friday to discuss further? Perhaps in the reco meeting if its on?

@jpata
Copy link
Contributor

jpata commented Jan 6, 2021

btw, unless I misunderstand, it seems to me that the bot status is not correct, as the tests were completed successfully (cc @smuzaffar)

@smuzaffar
Copy link
Contributor

@jpata , the branch used for this PR is also used for #32520 ( for which tests are not yet run). The bot status for this PR is update to date. The pending bot/32520/jenkins status is for the other PR.

@jpata
Copy link
Contributor

jpata commented Jan 6, 2021

The pending bot/32520/jenkins status is for the other PR.

Thanks for the clarification!

Sam-Harper pushed a commit to Sam-Harper/cmssw that referenced this pull request Jan 6, 2021
@fwyzard
Copy link
Contributor

fwyzard commented Jan 6, 2021

@Sam-Harper

[...] the HLT does wish (and to my knowledge does if not we should address this asap) to have a test of all code the HLT uses in jenkins and relvals via a periodic dump of the HLT.

Well, not all the modules ever implemented for the HLT are tested in the current "GRun" menu (from a quick look at the code and at the menu, I'd say around 100 or so are no longer used).
Are we suggesting that somebody should create a working test configuration for those 100 modules ? And maintain it for all future releases of CMSSW ?

Do all (other) RECO modules have that ?

@silviodonato
Copy link
Contributor

@cms-sw/hlt-l2 can we merge this PR?

About the discussion on the validation, we might also use the ORP meeting if you prefer.

@Martin-Grunewald
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jan 8, 2021

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 now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@silviodonato
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