Fixing a BS Issue affecting the Egamma HLT #19884
Conversation
|
A new Pull Request was created by @Sam-Harper for master. It involves the following packages: RecoEgamma/EgammaElectronProducers @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
is there a link to slides or HN for this? |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
please test |
|
@Sam-Harper |
|
@slava77 the slides are coming. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
| @@ -147,9 +154,16 @@ TrackingRegionsFromSuperClustersProducer(const edm::ParameterSet& cfg, | |||
| deltaPhiRegion_ = regionPSet.getParameter<double>("deltaPhiRegion"); | |||
| deltaEtaRegion_ = regionPSet.getParameter<double>("deltaEtaRegion"); | |||
| useZInVertex_ = regionPSet.getParameter<bool>("useZInVertex"); | |||
| useZInBeamspot_ = regionPSet.getParameter<bool>("useZInBeamspot"); | |||
| useDefaultZ_ = regionPSet.getParameter<bool>("useDefaultZ"); | |||
There was a problem hiding this comment.
useDefaultZ_ is never used as such in the code, and it looks redundant here. And, by the way, it is not even the "default", as it is false by default in the fillDescriptions method.
If you get rid of it, the check implemented now in validateConfigSettings() can simply become
if (useZInVertex_ && useZInBeamspot_) throw ...
And, to follow the flow that doesn't use neither the vertex or the beamspot one can just (more intuitively) set both those two flags as false.
There was a problem hiding this comment.
Agreed. I tried to think of a justification for my choice here and I found none. This is better
| return GlobalPoint(bsPos.x(),bsPos.y(),bsPos.z()); | ||
| //return GlobalPoint(bsPos.x(),bsPos.y(),0); | ||
|
|
||
| if(useZInBeamspot_){ |
There was a problem hiding this comment.
The comment at line 257-258 is not true any more: now if the vertex collection is empty we fall back to "default" (not beamspot) mode.
Depending on what was your original intention, either modify that comment, or modify the code accordingly (e.g. by defining another bool "verticesHandle->empty()" and put it in OR at L262)
There was a problem hiding this comment.
I hate silent fall backs and not sure how this one slipped past me. An exception will now be thrown if the vertex collection is not there and we were expecting it to be. Thanks for the spot
| @@ -181,6 +195,11 @@ fillDescriptions(edm::ConfigurationDescriptions& descriptions) | |||
| desc.add<double>("deltaPhiRegion",0.4); | |||
| desc.add<double>("deltaEtaRegion",0.1); | |||
| desc.add<bool>("useZInVertex", false); | |||
There was a problem hiding this comment.
You coud add a comment here explaining that "useZInVertex" and "useZInVertex" cannot be both true, see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#Adding_Comments
| @@ -181,6 +195,11 @@ fillDescriptions(edm::ConfigurationDescriptions& descriptions) | |||
| desc.add<double>("deltaPhiRegion",0.4); | |||
| desc.add<double>("deltaEtaRegion",0.1); | |||
| desc.add<bool>("useZInVertex", false); | |||
| desc.add<bool>("useZInBeamspot", true); | |||
| desc.add<bool>("useDefaultZ", false); | |||
There was a problem hiding this comment.
As already said, I would remove this parameter as it is both useless and confusing
| if(useZInBeamspot_) nrSetTrue++; | ||
| if(useDefaultZ_) nrSetTrue++; | ||
| if(nrSetTrue!=1){ | ||
| throw cms::Exception("InvalidConfiguration") <<" when constructing TrackingRegionsFromSuperClustersProducer there much be exactly one of useZInVertex(="<<useZInVertex_<<") useZInBeampot(="<<useZInBeamspot_<<") useDefaultZ(="<<useDefaultZ_<<") set"; |
There was a problem hiding this comment.
(Very minor, but in case you touch this PR you can fix "much" -> "must" in the text...
| @@ -181,6 +195,11 @@ fillDescriptions(edm::ConfigurationDescriptions& descriptions) | |||
| desc.add<double>("deltaPhiRegion",0.4); | |||
| desc.add<double>("deltaEtaRegion",0.1); | |||
| desc.add<bool>("useZInVertex", false); | |||
| desc.add<bool>("useZInBeamspot", true); | |||
There was a problem hiding this comment.
You could add a comment here explaining that "useZInVertex" and "useZInVertex" cannot be both true, see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#Adding_Comments
cmsbuild
added
comparison-pending
and removed
comparison-available
tests-approved
labels
|
Pull request #19884 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again. |
|
please test |
|
The tests are being triggered in jenkins. |
|
please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
tests-pending
tests-started
and removed
tests-started
tests-pending
labels
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
So I see there is a patch release needed for the muons. Could this PR be a part of that please. It does no harm, it gives identical results with existing configs It also appears on MC to do exactly what I intend it to do You would have data plots by now but the grid had issues last night and then RAL computing died today |
|
+1 |
|
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 requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
merge |
|
A little late but here are the slides requested: You would be most interested in slide 7 and 17-21 |
Dear All,
The online beamspot issues persist and this is impacting data taking efficiency for electrons. As such I have added more flexibility to the E/gamma code to deal with this. We now have various options available to us for tracking regions.
The TSG will decide which one we use, however having the options of all 4 are useful.
We really need this online as soon as possible as we are losing efficiency in electron triggers in a nasty way which is impossible to detect using t&p.
This code does not run at reco, it is HLT exclusive so far. It may well run in RECO in the future.
Validation:
Will run over data as soon possible but this is more the TSG, the MC tests show the code is functioning as expected.
Passes the scramv1 b run tests and runTheMatrix.py -l limited -i all
@Martin-Grunewald