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
Add full5x5 shower shape block to non-GED photons producer #12085
Add full5x5 shower shape block to non-GED photons producer #12085
Conversation
A new Pull Request was created by @richard-cms (R. Alex Barbieri) for CMSSW_8_0_X. Add full5x5 shower shape block to non-GED photons producer It involves the following packages: RecoEgamma/EgammaPhotonProducers @cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks. |
Are there non-ged photons that are based off of particle flow clusters? If (Sent from my Nexus 6)
|
There were no errors reported during workflows 140.2 or 33.0. The actual patch is simply copying the relevant block from PhotonProducerGED, which was added in this commit: 934681b |
@lgray |
Non-pfcluster-based photons do not use fractions, there is no way to even know about or reconcile with them. Pf and other dedicated em clusterings are incredibly different. The difference you saw comes from something else, or your explanation of the workflow is incorrect. It's likely inconsistent RecHit flagging or something similar. I encourage that you understand this a bit more before filling these variables with something unknown. |
@lgray, okay we'll try to track down what's actually going on here. |
@lgray However, we really do a difference in the sigmaIetaIeta values. Here's a short comparison on 5 events:
If I take a look in the bowels of the island clustering algorithm, I see lines like this one: What could possibly be different between these two sigmaIetaIeta calculations to cause this difference? |
@cmsbuild please test |
The tests are being triggered in jenkins. |
@@ -359,6 +359,19 @@ void PhotonProducer::fillPhotonCollection(edm::Event& evt, | |||
float sigmaIetaIeta = sqrt(locCov[0]); | |||
float r9 =e3x3/(scRef->rawEnergy()); | |||
|
|||
float full5x5_maxXtal = noZS::EcalClusterTools::eMax( *(scRef->seed()), &(*hits) ); |
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.
Before dereferencing hits, please check it is non-null. There is a possibility that hits would equal 0.
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 code was copied directly from https://github.com/CmsHI/cmssw/blob/forest_CMSSW_7_5_3_patch1/RecoEgamma/EgammaPhotonProducers/src/GEDPhotonProducer.cc#L502 which also does not checkout for null so I didn't think about this too hard.
I can add a non-null check to this file, but I'm loathe to change anything in GEDProducer to prevent PR creep.
@lgray : |
We spoke with Lindsey/Matteo some offline and are currently attempting some tests to get to the bottom of why the full5x5 and non-full5x5 calculations have differences. We will hopefully have some answers this week. |
Okay, I believe I have found the source of the discrepancy between the noZS (full5x5) and ZS (non-full5x5) results, and it is in the "getFraction" EcalClusterTools function: https://github.com/cms-sw/cmssw/blob/CMSSW_8_0_X/RecoEcal/EgammaCoreTools/interface/EcalClusterTools.h#L257 If noZS is false (non-full5x5), then the getFraction function returns '0' for rechits that it cannot find. The noZS version returns 1.0 in those cases (is this a bug, or intended?). 'getFraction' is called by matrixEnergy, and matrixEnergy is in turn called by e5x5, and the difference in e5x5 causes the difference in sigmaIetaIeta. If I force getFraction to return 1.0 always (the noZS behavior) the noZS and ZS results are the same. Unfortunately, I do not know if this behavior is a bug or is intended. @lgray or @matteosan1 do you have any ideas on:
Any thoughts are appreciated. @yenjie, and @ttrk are also interested. |
@cmsbuild please test |
The tests are being triggered in jenkins. |
} | ||
if(!hits) continue; |
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.
@richard-cms: Better style would be if (hits == 0)
, since hits is not a boolean, but this way is OK, too.
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.
Okay I switched it to hits == 0.
+1 For Heavy Ions, adding full5x5 shower shape to non-GED photons. #12086 and #12087 are the 75X and 76X versions of this PR, and the former has already been approved by Reco. The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2015-11-04-1100 show no significant differences, except for the expected addition of the shower shapes. |
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar |
+1 |
Add full5x5 shower shape block to non-GED photons producer
HI noticed that for photons produced with the "standard" photon producer, the full5x5 shower shape info was missing.
HI absolutely needs this info for photon ID purposes in run2 analyses, as we plan on using "standard" photons for run2 analyses.
@ttrk @mandrenguyen @yetkinyilmaz
We would like to request that this be included in the release used for HI data taking, potentially 7_5_3_patch2 or something similar. Apologies for not catching this error earlier.