-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Correcting a JEC indexing bug for the pat::Jet class in association to scaleEnergy function calls #36559
Conversation
… not correctly updated. The scale is added to the JEC collection onto the index 0, so this will always affect the correct index for the current JEC level. This commit provides a simple correction for the issue.
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36559/27503
|
A new Pull Request was created by @errai- (Hannu Siikonen) for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
There are no edits to scaleEnergy in the smeared MET PR, so there are no direct conflicts - and an edit to scaleEnergy is necessary to fix the bug at hand in this thread. However, as also the MET PR increments to pat::Jet and jet smearing introduces scaleEnergy calls, it seems worthwhile for me to take a moment to check the changes introduced in this MET smearing PR more deeply. |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-38ac16/21430/summary.html Comparison SummarySummary:
|
@errai- were some differences actually expected in the persisted data? |
@@ -249,6 +249,7 @@ void Jet::scaleEnergy(double fScale, const std::string& level) { | |||
if (jecSetsAvailable()) { | |||
std::vector<float> factors = {float(jec_[0].correction(0, JetCorrFactors::NONE) / fScale)}; | |||
jec_[0].insertFactor(0, std::make_pair(level, factors)); | |||
++currentJECLevel_; |
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.
this method Jet::scaleEnergy
was added in #27428 with an intention to fix calls like this https://github.com/cms-sw/cmssw/blob/CMSSW_12_1_0/DataFormats/PatCandidates/interface/Jet.h#L407-L410
Now that you are bringing up also possible repeated calls of scaleEnergy
, I'm not sure that this implementation which always inserts a new factor is going to do the right thing.
Is the information about the multiple scales needed/required?
- If not, then (somewhat unsafe, but still) assuming that only
scaleEnergy
inserts alevel=0,flavor=NONE
correction at a set 0, should this code instead of repeated insertion accumulate the product? - Alternatively, if the information about multiple scalings is important then the call to
chargedEmEnergyFraction() const {return chargedEmEnergy() / jecFactor(0) * energy());}
would be incomplete becausejecFactor(0)
only looks at a set=0, while in this context it should loop over all sets and get a product oflevel=0,flavor=NONE
in all available sets
@cms-sw/jetmet-pog-l2 @laurenhay @ahinzmann please check/clarify.
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.
If I understood the second question correctly, I might add a quick comment on it. On line 250 each new scale is added relative to the previous index zero scale. Cumulatively this indicates that jecFactor(0) * energy()
always gives the jet energy where all JECs and all scaleEnergy scales are undone. The functions chargedEmEnergy()
et al. refer to the jet energy composition before all jet energy corrections and scales, so this is sensible.
The first question is open for political discussion. In well-optimized code there should be no need for more than two calls to scaleEnergy, so there is not much difference. There might be some use-cases where both of the scales are needed separately. On the other hand, not so-well optimized code could abuse scaleEnergy calls.
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.
If I understood the second question correctly ...
Thanks for the clarification.
I missed somehow the accumulation of /fScale
from the previous state.
So, I'd say that no changes are needed here. The first case above is not that essential.
The expected output of the tests was actually unknown territory. Because the bug was there since July 2019, it was expected that the tests from back then didn't catch the issue, but there could have been new test developments in-between. Before the bugfix, each call to scaleEnergy() would create an offset by one to he current index (currentJECLevel_) w.r.t. the vector indices stored in the member jec_. Generally, scaleEnergy is used at least in jet smearing in MC, and likely also if systematic energy variations are applied in MC. The trick is that in MC, the L3Absolute and L2L3Residual jet energy corrections are unitary dummies in MC. Thanks to this, if the current (MC) JEC level was set to L3Absolute, the bug would only appear after two or more calls to scaleEnergy. And similarly, if the current MC JEC level was set to L2L3Relative, the bug would only appear after three or more calls to scaleEnergy. If the tests do not include involved scenarios such as systematical variations, they simply would not bring up the issue. It is most likely to come up in private (full) analyses with fully blown-up systematics studies. |
tests include standard miniAOD production for data (run2 so far has miniAOD, around 1K events over several workflows) and MC (run2 and run3, but less events per workflow). |
Thanks for the clarification! In my understanding the generic miniAOD workflow shouldn't be doing calls scaleEnergy, as it only produces the generic slimmedJets collection. The analyzers are expected to apply jet smearing and jet energy variations manually or via central tools by themselves. Both of these are easiest and most likely done by utilizing the scaleEnergy function. Thus, the issues materialize only late in the analysis chain. On a slightly related note: I'm not sure how and what is done in the nanoAOD workflow, and there were mentions that also this should be checked. |
there are nanoAOD workflows in the tests as well and at least the reco monitoring is of the nanoAOD products is a bit more inclusive (all values in a list of hardcoded tables are used for comparisons). |
Before signing, I'd still like to understand if the standard miniAOD/nanoAOD outputs are affected. Perhaps as a brute force check it's worthwhile to add a temporary commit that throws an exception in |
@slava77 I have already ran this command before submitting the PR on lxplus within the CMSSW environment that contains the patch (as it was a part of the suggested standard tests). There were no errors back then. Do we expect different results if I run this command again? Or would I need to perform some other modifications and then run the command again? |
When running tests multithreaded on miniAOD for wf 28234.0 (TTbar 14 TeV 2026D60) in PR #36568 there is hint of some non reproducibility of the MET corrections: I don't think it has anything to do with this proposed fix of the Jet energy corrections. But perhaps in this thread there is the expertize to evaluate it. Let me try to re-run threaded tests here, to see if it can reproduce in another PR. |
enable threading |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-38ac16/21452/summary.html Comparison SummarySummary:
|
@slava77 my bad for responding in a rush. If the info is still needed, the run with a scaleEnergy error thrown ended up with
so failures were indeed produced. I made a second test with a counter (originally initialized to zero) condition:
and now the printout was
That is, there seem to be maximally one scaleEnergy call - perhaps from jet smearing - in these tests. As long as the call was done to MC only, there is no reason to expect changes. If scaleEnergy was for some reason called for data, this would immediately cause problems with L2L3Res, but such calls seem unlikely. Hence the hypothesis stands: the scaleEnergy bug is most likely materialized in private analyses, performing e.g. jet energy scale variations. In the central workflow, it luckily seems to remain "silent". |
Thank you for checking explicitly. |
@errai- |
Within the pat::Jet class, scaleEnergy modifies std::vectorpat::JetCorrFactors jec_ and the current p4 through the function setP4, inherited from LeafCandidate ParticleState m_state. Moreover, it should modify currentJECLevel_, as was proposed in this fix. To the vector jec_ (within class pat::Jet) at index zero JetCorrfactors object the new scale is added. Here, the index zero refers to the jecSet index (not to be confused with jecLevel and currentJECLevel_). It is possible that instead of zero, one should use the index currentJECSet_. Typically there is only one set, and this does not make a difference. However, the intricacies of currentJECSet_ or 0 usage are beyond my understanding. Within the index zero JetCorrFactors, all the scales reside within (class JetCorrFactors) std::vector jec_. The factor for undoing the new scale is appended into the beginning of this vector, thus increasing the index of the current factor by one. The pat::Jet index currentJECLevel_ refers to the position within this vector. @slava77 let me know if this answers your question. The indexing and vectoring is split between the pat::Jet and pat::JetCorrFactors classes, adding a bit complexity. I'd guess that this is a result of making small increments to the classes over the years, and avoiding refactoring to preserve full backwards compatibility. |
@errai- |
@slava77 in my understanding slimmedJets should not be affected. Jet smearing is recommended practically for all MC samples, but it should only be run by the end user. The same goes for JEC systematics variations. It's a mystery to me, where the scaleEnergy call is produced. |
As far as I remember, there is only one occasion where scaleEnergy is used in official production namely in the MET uncertainties stored in MiniAOD+NanoAOD, one of them is JER-smearing another JES. As this is calling scaleEnergy only once, the bug may not affect MiniAOD+NanoAOD content. |
@cmsbuild please test with cms-sw/cms-bot#1685 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-38ac16/21564/summary.html Comparison SummarySummary:
|
+reconstruction
Based on this, it seems safe to also backport this feature, but I'd leave it to @cms-sw/jetmet-pog-l2 to confirm that this has to be backported. |
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 will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
PR validation:
if this PR is a backport please specify the original PR and why you need to backport that PR: