-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[EGM] Eta-Extended-Electrons (EEE) #35309
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25327
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25328
|
A new Pull Request was created by @swagata87 (Swagata Mukherjee) for master. It involves the following packages:
@Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b20406/18675/summary.html Comparison SummarySummary:
|
@@ -17,6 +17,24 @@ | |||
# pset.minGoodStripCharge = cms.PSet(refToPSet_ = cms.string('HLTSiStripClusterChargeCutNone')) | |||
# return process | |||
|
|||
# Eta Extended Electrons | |||
def customiseForPRNUM(process): |
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.
def customiseForPRNUM(process): | |
def customiseFor35309(process): |
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.
Please change PRNUM
to 35309
, here and below (only a technicality, but necessary..).
@@ -134,5 +152,5 @@ def customizeHLTforCMSSW(process, menuType="GRun"): | |||
|
|||
# add call to action function in proper order: newest last! | |||
# process = customiseFor12718(process) | |||
|
|||
process = customiseForPRNUM(process) |
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.
process = customiseForPRNUM(process) | |
process = customiseFor35309(process) |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35309/25357
|
Pull request #35309 was updated. @Martin-Grunewald, @jpata, @cmsbuild, @slava77 can you please check and sign again. |
if not hasattr(esp, 'MinNumberOfHitsHighEta'): | ||
esp.MinNumberOfHitsHighEta = cms.int32(5) | ||
|
||
return process |
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.
If these added parameters are all in their respective fillDescriptions method, then this customisation would not be needed. Is this the case or could it be achieved?
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.
It doesn't seem straightforward to me, as fillDescriptions function is not present in one of the codes where we added new parameter.
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.
Which one? Could it be added there?
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.
so this part can be deleted:
for esp in esproducers_by_type(process, 'KFFittingSmootherESProducer'):
if not hasattr(esp, 'HighEtaSwitch'):
esp.HighEtaSwitch = cms.double(5.0)
if not hasattr(esp, 'MinNumberOfHitsHighEta'):
esp.MinNumberOfHitsHighEta = cms.int32(5)
because TrackingTools/TrackFitters/plugins/KFFittingSmootherESProducer.cc
already have fillDescriptions
, and our new parameters are in there.
But the other part, the following, is needed:
for pset in process._Process__psets.values():
if hasattr(pset,'ComponentType'):
if (pset.ComponentType == 'CkfBaseTrajectoryFilter'):
if not hasattr(pset, 'highEtaSwitch'):
pset.highEtaSwitch = cms.double(5.0)
if not hasattr(pset, 'minHitsAtHighEta'):
pset.minHitsAtHighEta = cms.int32(5)
because new parameters are added in TrackingTools/TrajectoryFiltering/interface/MinHitsTrajectoryFilter.h
, and there is no fillDescriptions
. Adding fillDescriptions
in MinHitsTrajectoryFilter.cc doesn't solve the problem. It's not clear where we need to add fillDescriptions
to make it work.
If possible, we would like to proceed with HLT customisation for now, and we will try to figure out fillDescriptions
later on together with tracking experts.
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.
Can you then please remove the 'KFFittingSmootherESProducer'
customisation as it is not needed?
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.
BTW, HLTrigger/Configuration/python/customizeHLTforCMSSW.py shows a merge conflict, so it needs to be updated anyway.
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.
My bad on the customisation @swagata87; when we discussed it offline, I missed that it was not needed for one of the modules.
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.
I've now removed KFFittingSmootherESProducer
customisation and resolved the conflict...
My bad on the customisation @swagata87; when we discussed it offline, I missed that it was not needed for one of the modules.
oh no problem!
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.
It's not clear where we need to add
fillDescriptions
to make it work.
From a quick look, it seems that the issue is the lack of a fillDescriptions
in CkfTrackCandidateMaker (which in turn would probably be implemented as a function in CkfTrackCandidateMakerBase). That being said, this looks like something beyond the scope of this PR.
+1 |
It looks like this happens only in Run2 data... |
Here are the results from the Validation of this PR:
|
thanks @akanugan for doing the validation, |
@swagata87 could you explain this a bit more?
Is it that the eta mismeasurement of a GSF track causes the new high-eta hit cut to be used, which creates a slight difference in the GSF collection that PF sees, even with |
Hi Joosep, |
+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) |
+1 |
This PR might have caused |
For my understanding, would this have been visible from the standard matrix tests somehow, in order to perhaps avoid this in the future? I couldn't see this error in the matrix logs at the moment. |
IBs include more tests than the short matrix. |
PR description:
This PR aims to improve electron reconstruction efficiency in high eta region, 2.5<|eta|<3.0.
Motivation: EEEs can be important for many analyses, one example is measurement of weak mixing angle using forward-backward asymmetry of DY events.
Currently, electron reconstruction efficiency sharply falls off after around |eta|=2.5. This is because GSF tracking tends to fail in high eta, as the outer tracker coverage ends and we are left with only the inner tracker layers. Two parameters were identified, whose cut-values need to be relaxed, to improve GSF tracking efficiency in high eta region; they are the following:
However, this change was made only for high eta electrons, keeping the parameters' cut values same in low eta.
Note that while we expect EEEs to enter 12_1 and MC samples to be produced with it, the PF Electron ID and energy regression for EEEs would likely be suboptimal. So a temporary switch is added to prevent EEEs to enter particle flow. When 12_1 samples will be available, we will retrain PF electron ID and energy regression including EEEs, and we will put in the updates in 12_2 while removing this switch and allowing EEEs to the particle flow. This is done as an intermediate, safe approach.
PR validation:
Some checks were made using 1000 double-electron gun events.
This plot shows electron eta distribution(from AOD); default CMSSW in blue and after merging this branch in green. More electrons are reconstructed in high eta, but there is no change in other regions.
This is general track eta distribution(from AOD), showing no change, as intended.
runTheMatrix.py -l 12434.0
ran fine (ttbar 2023)This PR is not a backport.
Backport not needed.
This work is done together with Shilpi and Yash; in collaboration with PF group (Josh, Kenichi, Kenneth et. al) and with help from tracking experts (Mia et. al). Thanks to Marino(HLT-STORM) for helping with HLT customisation.