ECAL PFCluster Correction 9X #19307
ECAL PFCluster Correction 9X #19307
Conversation
|
A new Pull Request was created by @rafaellopesdesa (Rafael Lopes de Sa) 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 |
|
hold |
|
Pull request has been put on hold by @franzoni |
|
We'll come to these GTs as soon as other integration of conditioned will be done ( may take 2-3 days ) which cannot be done in parallel |
|
Thanks, @franzoni |
|
@kmcdermo please check if this will affect your work for running OOT photons from legacy miniAOD. |
| bool iseb = cluster.layer() == PFLayer::ECAL_BARREL; | ||
| if (iseb){ | ||
| EBSrFlagCollection::const_iterator srf = ebSrFlags->find(readOutUnitOf(static_cast<EBDetId>(cluster.seed()))); | ||
| clusFlag = srf->value(); |
There was a problem hiding this comment.
is it logically possible for the find to find nothing and srf to be invalid and impossible to dereference?
If so, there better be an appropriate protection.
| else { | ||
| if (pt<2.5) coridx = 1 + regind; | ||
| else if (pt>2.5 && pt<6.) coridx = 2 + regind; | ||
| else if (pt>6.) coridx = 3 + regind; |
There was a problem hiding this comment.
please make sure that there are no gaps (values with pt==2.5 and 6 are not covered.
| coridx += 5; | ||
| if (clusFlag!=1 && clusFlag!=3) { | ||
| edm::LogWarning("PFClusterEMEnergyCorrector") << "We can only correct regions readout in ZS (flag 1) or FULL readout (flag 3). Flag " << clusFlag << " is not recognized."; | ||
| edm::LogWarning("PFClusterEMEnergyCorrector") << "Assuming FULL readout and continuing"; |
There was a problem hiding this comment.
What are the other options and how often can they happen?
If they can happen normally for some data types, the warning should not be issued in this case.
Also, please use only one LogWarning. << "\n" or << std::endl can be used for multiple line warning
|
|
||
| // Form ietamod20 and iphimod20 as well | ||
| int signeta = (ietaix > 0) ? 1 : -1; | ||
| int ietamod20 = (std::abs(ietaix) < 26) ? ietaix - signeta : (ietaix - 26*signeta) % 20; |
There was a problem hiding this comment.
what are "26" and "20" in this context? Are they available from some header or geometry as constants?
| << size << " " << reducedHits; | ||
| LogDebug("PFClusterEMEnergyCorrector") << "isEB : eraw : ePS1 : ePS2 : (eps1+eps2)/raw : Flag = " | ||
| << iseb << " " << evale << " " << ePS1 << " " << ePS2 << " " << (ePS1+ePS2)/evale << " " << clusFlag; | ||
| LogDebug("PFClusterEMEnergyCorrector") << "response : correction = " |
There was a problem hiding this comment.
this can be sent over just one LogDebug instead of 3
|
@rafaellopesdesa |
|
@rafaellopesdesa |
|
We can maintain the old correction for 2016 era, if need be (perhaps it's the easiest way). Specially because the PF selective readout was not implemented last year. If you don't like this option... then maybe we need some help. The truth is that we haven't tested these corrections with 2016 era setup. We didn't think we would be generating 2016 MC with these releases. We can generate some MC with 2016 era setup to test. Do you have an example of workflow that I can run with the 92X release for this test? That would help us. |
|
Well, the first would be to modify the code to ensure it does not crash on missing the srFlags, which are not present in 80X AOD. I had to do something similar in #19404 (some reason it is not linking: #19404), see: Then, for sure, maintaining the old correction would be great. As for 2016 MC in 92X, we are actually trying to debug some things ourselves first (with our signals in 2017). @lsoffi and I are trying to figure this out... we can get back to you shortly on this. |
|
@kmcdermo Sorry, I think I was not clear. This will not be ported to 8X. 8X is not a concern here. This is a correction only valid for 9X, where SR flags are available and important (since the PF ZS and SR were finally implemented) |
|
What would really be a problem is if you try to use this correction with 2016 era.... if I re-instante the old correction, it will be for 2016 era in 9X only. Not 8X, this will remain untouched. @slava77 : About the other code review, @jainshilpi is looking at them and will commit the fixes. |
|
@rafaellopesdesa Yes, indeed, no need to backport this. I am saying that, however, the code as it stands will crash on not having the srFlag products at AOD. So if we can add a protection to the code to prevent this in 92X, that would be best. |
|
And indeed, just re inserting the old correction for 2016 is enough. |
|
@kmcdermo Still not understanding: This code will never run over AOD (the SR flags is the least of the concerns). Why would you use a 9X release to run over 8X RAW? Will CMS do that? |
|
@rafaellopesdesa ah sorry for not explaining. In the 80X re-miniAOD, which will start from AOD, we will be adding in a new collection (out-of-time photons). Since this collection does not already exist, we have to go through the full reco chain for this particular collection, which includes calculating the PFClusterIsolation. |
|
I don't know for certain what depends on what in the code and conditions... indeed the "right" solution is that all numbers like this one come from the conditions (as to have everything in one place so its easy to be self-consistent with both the current and the past). I suspect thats hard. Right?
Assuming so - are the string names (eg ecalPFClusterCorV2_EB_pfSize1_mean_50ns) not enough to define which set of magic pt cut values are relevant for building the index into the corrections objects?
… On Jul 3, 2017, at 10:44 AM, Rafael Lopes de Sa ***@***.***> wrote:
@davidlange6 Sorry, I do not understand very well your suggestion. Where would this look up be defined? At the parameters (cfi, cff) file? At the conditions database itself?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@davidlange6 Yes, getting the binning from the conditions DB would require a lot of changes at this point. As for the name, that's basically what is being done in the code. Different options are controlled by the options applyMVACorrections and srfAwareCorrection. The user selects these options and the code will correctly know about the tag names and binning option. But I agree that it introduces code complexity... I just don't know what to do as an alternative at this point. |
|
@davidlange6 @rafaellopesdesa |
|
@arunhep I am waiting instructions of whether or not the current version of the code is acceptable. |
|
we are waiting for this PR to get merged so that we can open last one for condition updates for 930pre1 build. |
|
@arunhep I understand, I am trying to have turn arounds as quickly as I can. |
|
On 7/3/17 3:01 AM, Rafael Lopes de Sa wrote:
@davidlange6 <https://github.com/davidlange6> Yes, getting the binning
from the conditions DB would require a lot of changes at this point.
IMO, based on the past changes, a new retraining will likely contain
different variables as well.
So, asking for conditions-based settings for bins will have a rather
limited utility.
A complete DB-driven change would require the code to be moved to DB.
… As for the name, that's basically what is being done in the code.
Different options are controlled by the options applyMVACorrections and
srfAwareCorrection. The used selects these options and the code will
correctly know about the tag names and binning option. But I agree that
it introduces code complexity... I just don't know what to do as an
alternative at this point.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19307 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbpjhOdhznTck0UjAKdkqDaabIs4Rks5sKLvygaJpZM4N9eXa>.
|
|
On Jul 3, 2017, at 2:36 PM, Slava Krutelyov ***@***.***> wrote:
On 7/3/17 3:01 AM, Rafael Lopes de Sa wrote:
> @davidlange6 <https://github.com/davidlange6> Yes, getting the binning
> from the conditions DB would require a lot of changes at this point.
>
IMO, based on the past changes, a new retraining will likely contain
different variables as well.
So, asking for conditions-based settings for bins will have a rather
limited utility.
A complete DB-driven change would require the code to be moved to DB.
nice - so we can conclude both of this and the PF conditions are tightly coupled to code and will break backwards compatibility well into the production release cycle
we have confirmed that old conditions throw an exception when being read by new code?
…
> As for the name, that's basically what is being done in the code.
> Different options are controlled by the options applyMVACorrections and
> srfAwareCorrection. The used selects these options and the code will
> correctly know about the tag names and binning option. But I agree that
> it introduces code complexity... I just don't know what to do as an
> alternative at this point.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#19307 (comment)>, or
> mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEdcbpjhOdhznTck0UjAKdkqDaabIs4Rks5sKLvygaJpZM4N9eXa>.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@davidlange6 The new and old conditions are controlled by configuration parameters. If you try to run the new calibration in an old sample it will run an exception because it will not find the necessary variables. The new calibration is activated via era mechanism. |
|
On Jul 3, 2017, at 2:45 PM, Rafael Lopes de Sa ***@***.***> wrote:
@davidlange6 The new and old conditions are controlled by configuration parameters. If you try to run the new calibration in an old sample it will run an exception because it will not find the necessary variables.
The new calibration is activated via era mechanism.
and the old calibration is still supported by the new code?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
@davidlange6 Yes, and the default for older eras... |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
merge |
|
Comparison is ready Comparison Summary:
|
Code for the 9X ECAL PFCluster corrections.
Updates include:
@perrotta and @slava77 : the code as it is will not pass validation because the tags consumed are not in the GT. The request to include in the global tag have been done [2]. I guess @arunhep or @franzoni will also have to point the "auto" GT to the ones with these tags before this code has any hope of passing validation (similar to discussions in [3])
Attention: @jainshilpi @fcouderc @Sam-Harper @arunhep @franzoni
[1] https://indico.cern.ch/event/646805/contributions/2627253/attachments/1477404/2289247/status_regression_15june.pdf
[2] https://hypernews.cern.ch/HyperNews/CMS/get/calibrations/3008.html
[3] #18912