Do not reset rechit flag inside the module #16006
Conversation
|
A new Pull Request was created by @igv4321 (Igor Volobouev) for CMSSW_8_1_X. It involves the following packages: RecoLocalCalo/HcalRecProducers @cmsbuild, @cvuosalo, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
|
@cmsbuild please test ... at some point it makes sense to combine in one PR |
|
The tests are being triggered in jenkins. |
|
Maybe for your convenience and speed of testing... But the three short PRs I recently submitted are all independent and logically disconnected. If one of them fails some tests or if we have additional discussion about it, it would be still nice to integrate the others as soon as possible. |
| @@ -554,7 +554,8 @@ void HcalHitReconstructor::produce(edm::Event& e, const edm::EventSetup& eventSe | |||
| auxflag+=((i->sample(fTS2).capid())<<28); | |||
| (rec->back()).setAuxHBHE(auxflag); | |||
|
|
|||
| (rec->back()).setFlags(0); // this sets all flag bits to 0 | |||
| // (rec->back()).setFlags(0); Don't want to do this because the algorithm | |||
| // can already set some flags | |||
There was a problem hiding this comment.
instead of keeping the old revision (code that was there since rev 1)
it would be more appropriate to just say something in line:
// set flags in addition to those (possibly) already set by the reco_ algorithm
Also, the original design was apparently more factorized: all flags are set in one place.
Can this be preserved in case some rework of hit flagging needs to be done?
(This is not a particularly strong requirement here though.)
Some rework of flagging has been among lingering topics to review/revisit for calo rechit.
|
In this particular case, I prefer to comment out the old code and make a note why it was commented out -- just in case somebody might think that it would be a good idea to reset the flags here. It is, indeed, possible to keep flagging in one place -- but that would require changes in the rechit itself because the information previously carried (very economically) by flags would have to be carried in a separate field. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
comparison-pending
tests-pending
tests-started
and removed
comparison-available
tests-approved
tests-pending
labels
|
type bugfix |
|
+1
E.g. 136.731 (2016B single photon PD) Apparently, nobody looked at this flag since it was introduced in May 2015. |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_1_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
+1 |
Bug fix: do not reset rechit flag inside the HcalHitReconstructor module. The flag may be already set. In particular, 1 or 3 pulse fit flag is set for "Method 2" by the algorithm creating the rechit.