Spike cleaner fix #17859
Spike cleaner fix #17859
Conversation
|
A new Pull Request was created by @knash for master. It involves the following packages: RecoParticleFlow/PFClusterProducer @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
@bachtis @bendavid @argiro @emanueledimarco |
|
@knash Is there a skim of events affected by changes in this PR? Thank you. |
|
Comparison is ready There are some workflows for which there are errors in the baseline: |
|
Hello, this fix was discussed in a DPG-PH JetMET meeting (https://indico.cern.ch/event/611068/) but without further details. This has been tested on a skim that exhibits anomalously energetic jets, of which one (out of ~30) was triggered (but for an ECAL spike). We wanted to start this now, but can put together a higher statistics study as well. This cleaning should be redundant to a similar spike cleaning cut that is already applied, so we do not expect much of a change. |
|
Hi, I am not very familiar with this piece of code. Ecal has its own flagging of spikes that I thought was used by PF. Perhaps @lgray , @Sam-Harper could comment ? |
|
@knash |
|
|
||
| int comp = std::abs(hitlayer-13); | ||
| const HcalDetId& detid = (HcalDetId)rechit.detId(); | ||
| std::vector<uint32_t> RawDetIds; |
There was a problem hiding this comment.
use lower case initial for local variables
There was a problem hiding this comment.
Thanks, this will be included in the next version
|
|
||
| //Fix needed for HF | ||
| float compsumE = 0.0; | ||
| if ((hitlayer==12 or hitlayer==11)) |
There was a problem hiding this comment.
Can these magic numbers be get from some configuration?
There was a problem hiding this comment.
This says to only apply the fix only to the HF layers. I think this should be hard coded, as any other values would be buggy
There was a problem hiding this comment.
11 -> PFLayer::HF_EM
12 -> PFLayer::HF_HAD
| int curphiL = hphi-predphi; | ||
| int curphiH = hphi+predphi; | ||
|
|
||
| if (predphi>abs(hphi) or predphi>abs(72-hphi)) |
There was a problem hiding this comment.
Can this "72" magic number be get from some configuration?
There was a problem hiding this comment.
This is a product of the HF valid phi numbering range (1-72). I think this needs to be hardcoded as well
There was a problem hiding this comment.
Yes, let it hardcoded. But please extract it from the "HF valid phi numbering range", whenever it is stored/defined. So that if it gets ever modified you don't have to modify several hidden lines of code here and there.
There was a problem hiding this comment.
My frame of reference for this range (and most of these methods) is here
Here, the "magic" numbers 39,4,72 are not from an exterior definition. I will look further however
| { | ||
| int predphi =2; | ||
|
|
||
| int comp = std::abs(hitlayer-13); |
There was a problem hiding this comment.
"13" here is quite likely "12+1", where "12" can be extracted from some enum or configured list
There was a problem hiding this comment.
Sure, this is just selecting the Hcaldetid depth which is a statement that the HF_EM companion energy is from the HF_Had and vice versa.
I will change this to an if statement as there are only two possible comp values
|
ping : @bachtis @bendavid @argiro @emanueledimarco @lgray @Sam-Harper |
| float compsumE = 0.0; | ||
| if ((hitlayer==12 or hitlayer==11)) | ||
| { | ||
| int predphi =2; |
There was a problem hiding this comment.
Hello, I would put the declaration below together with the condition under which predphi is set to 4.
All in all, I cannot understand what this code is doing, would be nice to add a few comments, in particular a general comment explaining the problem you want to solve and how you can solve it ("Fix needed for HF" is too generic). When I understand better, I'll be able to comment further.
There was a problem hiding this comment.
Ok, in the next version there will be more verbose comments.
This section checks if a rechit is from the HF region and if so it adds additional energy from the companion rechits (ie HF_Had <-> HF_EM) to the total surrounding energy
|
Comparison is ready Comparison Summary:
|
|
+1 |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
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 |
|
@fwyzard: Hello, should I be attempting these HLT tests or is there an expert that should do this instead? There should be exactly no change from this, so I think that the comparison should be pretty straight forward and also a good closure test. |
|
As I've written earlier
.A |
|
Any progress on this matter? |
|
Hello, I have been attempting to contact the experts. Perhaps there is an example config that compares an in-development HLT menu? |
|
By the way, @Martin-Grunewald : after the latest updates in the HLT menu, does this PR still affects it? Isn't it already disabled ECAL/HF spyke cleaning in it? If so (but please confirm) then we can probably just merge this PR and use the forthcoming results of the performance studies to decide whether to re-enable it in HLT. |
|
All instances of PFClusterProducer have the recHitCleaners empty, so from this side all is done. |
|
Thank you Martin.
Then, if I understand it correctly, this pull request can be finally
merged: are there other remaining issues @davidlange6 ?
Martin Grunewald <[email protected]> ha scritto:
… All instances of PFClusterProducer have the recHitCleaners empty, so
from this side all is done.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#17859 (comment)
|
|
Hello, quick question. As a side note, this PR should not be merged unless the spike cleaner is taken out of the HLT at the same time, but from what I can see, the only changes were to the ECAL clusters. Since this has already been tested, is there any point in running the HLT test? |
|
@knash : you can check the menu currently dumped in cmssw in If you do so, you can realize that the ECAL cleaning is still included in the PFClusterProducer hltParticleFlowClusterECALUncorrectedForMuonsMFNoVtx, see recHitCleaners in that module instantiation. This was a bug, fixed in the HLT menu n ConfDB. The HLT menu now in CMSSW is /dev/CMSSW_9_2_0/HLT/V347: the bug is correctly fixed in the latest version in the master, /dev/CMSSW_9_2_0/HLT/V365 (it was already fixed since V349, indeed) |
|
@perrotta: Thanks, ok so then you mean https://its.cern.ch/jira/browse/CMSHLT-1242 did not remove all of the spike cleaning as it should have? My issue was that there is no HF spike cleaning, only ECAL. The ECAL portion is tested in a separate in a separate PR (#18438). Shouldn't this HLT test be performed in that PR identical to the original tests? |
|
I state again: in the recent HLt menus, the recHitCleaners PSet is EMPTY for all instances of PFClusterProducer. Also, as far as I know, the question was never about ecal spike cleaning - that removal was uncontroversial. This issue is/was only about the HF. |
|
+1 |
|
@knash |
|
@Martin-Grunewald Hello, Could you elaborate a bit? I'm not sure exactly the logic that goes into this decision. If it helps, this PR itself should not affect data taking whatsoever. There is a small effect from the ECAL double spikes, but that is already implemented |
|
Yes, but MC production is done with 92X. |
|
Hmm, well there was interest in testing this cleaning for the current iteration, so I suppose this should be backported. |
|
Generally would be good to have all the bug-fixes in this round of MC as well. However: Any "particle flow noise filter" re-design would be done for the 2018 so that we can better use this HF detector up to the 2025! Maria |
|
@mariadalfonso |
Comparing the current HF PF spike cleaning recipe to that seen in 1 (slide 17), there seems to be a bug in the code such that the cleaning never occurs (would require a ~10^10 GeV spike) due to the fact that the rechit energy is summed in the numerator and denominator (L123,L137 of the original).
This PR attempts to fix the issue by explicitly finding the corresponding 9 neighbors (<=5 from L120 to L164) and the other <=4 from the loop starting at 171.
The code also would affect ECAL spikes which use the same cleaner. There, the changes only take out the rechit energy from the numerator without any additional summing required. This can also be motivated 1 (slide 8)