Factor of 2 bugfix for HF PF rechits w/ short-fiber only #38464
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38464/30676
|
|
A new Pull Request was created by @mandrenguyen (Matthew Nguyen) for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c5fe37/25694/summary.html Comparison SummarySummary:
|
|
Thanks @mandrenguyen. Can you also make a back port to 12_4, as discussed in the PPD meeting? |
|
urgent |
|
Sorry to contribute entropy here, but maybe it's worth a quick comment in the code saying why you are dividing the factor of two? If someone really cares they can find it here, but it would be consistent with the rest of the code to have an in-line comment (just the first 1-2 sentences you have here in the PR). If this slows down the integration of the PR don't worry about it. |
|
+reconstruction
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
reconstruction-pending
pending-signatures
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 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) |
|
I think that rewriting L143-L168 of the code as follows would give identical results and fix the bug, but it could be more intuitive and avoid the (correct) suggestion of adding some inline documentation by @kdlong here above double energy = sHORT;
if (found_hit != tmpOut.end() && found_hit->detId() == longID.rawId()) {
double lONG = found_hit->energy();
//Ask for fraction
//If in this case lONG-sHORT<0 add the energy to the sHORT
if ((lONG - sHORT) < thresh_HF_)
energy += lONG;
else
energy += sHORT;
if (abs(detid.ieta()) <= 32)
energy *= HFCalib_;
newHit.setEnergy(energy);
if (!(sHORT > shortFibre_Cut && (lONG / sHORT < longFibre_Fraction)))
if (energy > thresh_HF_)
out->push_back(newHit);
} else {
//only short hit!
if (abs(detid.ieta()) <= 32)
energy *= HFCalib_;
newHit.setEnergy(energy);
if (energy > thresh_HF_)
out->push_back(newHit);
}What do you think @mandrenguyen @clacaputo etc? |
was too quick, this is the fix suggested by Lev code format reorganize code in more logical way
mandrenguyen
force-pushed
the
shortFiberHF_bugfix
branch
from
c505ed1
to
9d533e3
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38464/30733
|
|
Pull request #38464 was updated. @jpata, @cmsbuild, @clacaputo can you please check and sign again. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c5fe37/25797/summary.html Comparison SummarySummary:
|
|
@cms-sw/reconstruction-l2 the logic of the algo didn't change since your last signature, and the reco/dqm outputs are identical. I merge this master PR in order to speed up the integration of the backport PR in 12_4_X. |
|
merge |
|
+1 |
cmsbuild
added
reconstruction-approved
fully-signed
and removed
reconstruction-pending
pending-signatures
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 will be automatically merged. |
PR description:
Bug-fix for a dropped factor of two in the PF HF rechit reconstruction. The energy is scaled by a factor of two in the code anticipating that the long and short fibers will be added. However, when there is only a deposit in the short fiber, this factor of two needs to be scaled back out.
Heavy-ions use low energy HF rechits for discriminating hadronic interactions from electromagetic and noise. This bug was found to reduce discrimination compared to using HF towers (which are not available at mini-AOD).
This was summarized at a recent PF meeting:
https://indico.cern.ch/event/1102173/contributions/4925583/attachments/2464753/4226434/HF_lk_17June_2022.pdf
We would certainty like to fix this for 12_5, where we use the HF essentially only for event selection.
It will be discussed at PPD tomorrow (June 23rd), whether this should also be backported for the upcoming high-lumi pp data.
PR validation:
if this PR is a backport please specify the original PR and why you need to backport that PR:
Bug-fix for a dropped factor of two in the PF HF rechit reconstruction. The energy is scaled by a factor of two in the code anticipating that the long and short fibers will be added. However, when there is only a deposit in the short fiber, this factor of two needs to be scaled back out.
Heavy-ions use low energy HF rechits for discriminating hadronic interactions from electromagetic and noise. This bug was found to reduce discrimination compared to using HF towers (which are not available at mini-AOD).
This was summarized at a recent PF meeting:
https://indico.cern.ch/event/1102173/contributions/4925583/attachments/2464753/4226434/HF_lk_17June_2022.pdf
We would certainty like to fix this for 12_5, where we use the HF essentially only for event selection.
It will be discussed at PPD tomorrow (June 23rd), whether this should also be backported for the upcoming high-lumi pp data.