GsfElectronProducer Reading off the end of a std::vector #38175
Comments
|
The full traceback from ASAN is |
|
Assign reconstruction. |
|
A new Issue was created by @Dr15Jones Chris Jones. @Dr15Jones, @perrotta, @dpiparo, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
|
assign reconstruction |
|
New categories assigned: reconstruction @jpata,@slava77,@clacaputo you have been requested to review this Pull request/Issue and eventually sign? Thanks |
cmsbuild
added
pending-signatures
reconstruction-pending
and removed
pending-assignment
labels
|
Looking at the code, it appears to start from this call in GsfElectronProducer Given it is 0 bytes beyond an array of size 12, I'm guessing that the |
|
So the underlying determination of which model to use (and thus how many outputs will be present) is decided by this piece of code cmssw/RecoEgamma/ElectronIdentification/src/ElectronDNNEstimator.cc Lines 12 to 34 in 0b44f88 In particular, line 30 where https://github.com/cms-sw/cmssw/blob/master/DataFormats/CaloRecHit/interface/CaloCluster.h#L181 What is worse, is line 63 is and since I can't find any |
|
type egamma |
|
Thanks Chris for the investigation! @valsdav @a-kapoor @swagata87 @cms-sw/egamma-pog-l2 please take note of this issue. |
|
Looking at it now. @jpata |
|
I think the underlying problem is with the interface presented by |
|
@Dr15Jones Thanks a lot for all your comments. I completely agree with all the general comments. The interface egammaTools::EgammaDNNHelper::evaluate can be improved significantly. I did some investigation, and to me it looks like the issue, rather, is inconsistent usage of el.superCluster()->eta() and el.eta(). The fact that there should be 3 nodes is decided based on el.eta(), when it should actually be decided based on el.superCluster()->eta(). Do you think this makes sense with what we see in the crash? @Dr15Jones Regarding using std::abs vs just abs, I printed out some values (while running the hlt_mc_PIon addon test) and it looks like the abs function is working fine Anyway, I also agree that we should still move to std::abs. I am working on a fix and checking if this inconsistency exists in other places of the DNN code. |
Anything that could cause the caller of the function and the function to disagree on which model was used would cause the crash. Numerical differences causing the cut logic to be different between the two would definitely cause it.
Very strange. How did you do the print comparison? https://godbolt.org/z/EWh9d5c8v NOTE: if somewhere in your test you have |
Okay! Here is a commit I created with just the cout statement in GsfElectronProducer.cc Maybe there is a "using std" hidden somewhere but it is not in GsfElectronProducer.cc. Could it be picking from somewhere else? |
Guess one of the included headers is bring something in. I didn't find any evidence it comes from CMSSW. But for now, it doesn't matter since you've determined it is NOT an int conversion going on. |
Indeed. I am working on fixing the actual problem and will get back on this thread. |
|
|
@Dr15Jones @jpata @swagata87 @lfinco |
|
+reconstruction |
cmsbuild
added
fully-signed
reconstruction-approved
and removed
reconstruction-pending
pending-signatures
labels
|
This issue is fully signed and ready to be closed. |
Running a PR test under ASAN showed the problem in the addOnTest hlt_mc_PIon
This appears to be related to the use of the DNN.
The text was updated successfully, but these errors were encountered: