-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
PFID with DNNs - updated model trained on Run3Summer21 #36243
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36243/26878
|
A new Pull Request was created by @valsdav (Davide Valsecchi) for master. It involves the following packages:
@jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36243/26893
|
@@ -57,6 +57,10 @@ class PFEGammaFilters { | |||
float ele_dnnLowPtThr_; | |||
float ele_dnnHighPtBarrelThr_; | |||
float ele_dnnHighPtEndcapThr_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these *Thr_
, as well as the ele_max*
and most of the badHcal_*
ones, can be made const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We kept them non-const only because they are read from a group of options in the constructor https://github.com/cms-sw/cmssw/blob/master/RecoParticleFlow/PFProducer/src/PFEGammaFilters.cc#L72 and I felt it would be ugly to put that in the initializer list. How do you suggest to fix this problem? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm, yes, it would require some gymnastic indeed...
Ok, don't mind
cc796ad
to
2325db7
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36243/26894
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment inline. let's run a test with profiling too.
@@ -15,14 +15,22 @@ | |||
particleFlowTmp.PFEGammaFiltersParameters.allowEEEinPF = cms.bool(False) | |||
|
|||
# Thresholds for e/gamma PFID DNN | |||
# Thresholds for electron: Sig_isolated+Sig_nonIsolated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be good to have a reference to an indico presentation here with these numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jpata, It should be this one https://indico.cern.ch/event/1096395/#12-egm-updates-for-run-3-prepa., shown at the last PPD meeting by @akapoorcern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, please put it in the code comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok! 👍
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d5bc9c/20787/summary.html Comparison SummarySummary:
|
@valsdav please summarize the CPU and MEM changes of this PR (initialization and processing) based on the profiles |
+reconstruction
|
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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
Thanks @jpata for the summary. |
Is mkFit here a typo, or was this comment meant for the mkFit PR? GsfElectronProducer is showing up among the stream modules, and in any case, the total CPU/mem looks also fine on this PR. |
PR description:
This PR updates the PFID DNN models trained on Run3Summer21 samples. A few additional variables have been included for electrons.
The paths of the models have been updated as well as the thresholds defining the default working point.
An additional threshold on the non-isolated background class has been included for electrons in PFEgammaFilters.
This PR comes along two PRs for ElectronIdentification (cms-data/RecoEgamma-ElectronIdentification#24) and PhotonIdentification (cms-data/RecoEgamma-PhotonIdentification#10) data repositories.
PR validation:
For documentation about the changes in the model training and input variables please refer to the slides at: https://indico.cern.ch/event/1096395/#12-egm-updates-for-run-3-prepa.
The correctness of the DNN configuration has been verified. A validation of the physics performance of the PFID WPs is ongoing in the PF and Egamma group.