Fix bug in sign of cot(alpha) in qbin() methods, needed for CPEGeneric errors #25195
Conversation
…d by the error calculation in PixelCPEGeneric
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-25195/7203 |
|
A new Pull Request was created by @pmaksim1 (Petar Maksimovic) for master. It involves the following packages: CondFormats/SiPixelTransient @ggovi, @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
type bugfix |
|
@pmaksim1 could you or someone from the Tracking POG provide the usual validation based on some larger sample than the automatically run jenkins tests? Link to a presentation in a POG or anywhere else will also be fine. Thank you! |
|
Sure, but it may take a bit. Would 10k events be enough? We established that there was a bug in the logic and that the strange errors observed by Vincenzo are gone. However we haven't studied the impact on other things. |
|
Thank you Petar.
Yes, indeed that's the point. The fix is deserved, but before
integrating it in the release one should verify that there are no
unwanted effects propagated downstream unwillingly, maybe because of
some tuning that was done by taking that bug implicitly into account.
Yes, some 10k single muon and some less TTbar events may be enough,
but I let you the expert decide about it.
Petar Maksimovic <notifications@github.com> ha scritto:
… Sure, but it may take a bit. Would 10k events be enough? We
established that there was a bug in the logic and that the strange
errors observed by Vincenzo are gone. However we haven't studied
the impact on other things.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#25195 (comment)
|
|
Copy that. We're on it. |
|
+1 |
|
Hi, We run a tracking validation workflow for TTbar events (10K) comparing with and without this PR. Plots are here: We have not find any significant differences yet but maybe someone may have a better intuition on this. |
|
Thank you @cmantill for providing the comparisons. A part a few fluctuations here and there, what looks sistematically different with respect to the baseline is that efficiencies and fakerates decrease at eta<-2 for very low pt (pt<200 MeV) tracks: there are no significant changes for pt>0.9,instead. Is this expected, given the fix applied? |
|
interesting: may I ask which workflow was used? it looks a phase0 one (which should be unaffected by the change). |
|
This was run using GT=auto:phase1_2017_realistic and with a TT workflow with runTheMatrix (runTheMatrix.py -e -n -l 10024.3) |
|
i.e. trackingOnlyRun2 (phase1 geometry, phase0 reco) |
|
OK sorry about that! submitted jobs now and will post results here asap. |
|
I posted comparison plots with this workflow here: http://cmantill.web.cern.ch/cmantill/pixel/validation_PR25195_Tracking/plots/ |
|
@cmantill : could you please check if the very same events were used for the two samples A and B? |
|
I think some of jobs did not finish, so I resubmitted them. Now both should have all stats and same # of tracking particles |
|
Latest comparisons attached in #25195 (comment) show just small fluctuations in the results, with no particular trend on them. As such the bug fix can be considered ready for being accepted. Before doing so: @VinInn @cmantill is the issue that was reported for the comparisons in #25195 (comment) understood by now? Perhaps also those comparisons could profit of being checked whether there were unfinished jobs which bias the result? Would it be worth to re-run them based on the very same events in the baseline and baseline+PR? Please let us know. |
|
Ping @VinInn @cmantill: could you please answer to my #25195 (comment)? |
|
is ok for what I am concerned, BUT I am not the official validator. |
|
Thank you Vincenzo! You are not the official validator, but the super-expert that wrote "-1" here above in this thread: I just wanted to be sure that there are no complains any more from your side ;-) |
|
+1
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
This PR fixes a longstanding software bug that was introduced when we implemented the Phase I FPix. The FPix template entries for IP related tracks are folded into one quadrant of positive (cotalpha, cotbeta) space and sign flips are used fix signed quantities. The subtlety is that one can get negative values (cotalpha, cotbeta) from cosmic rays that must also be included. Therefore the flipping is governed by flags that depend on the local magnetic fields. The different cases are handled by the signs of cota/cotb and the flags flip_x/flip_y. This is done correctly in the interpolation code for the template reco, but in the qbin method (used for generic reco) in the template and generror code, the reference to cotalpha was not changed to cota [sign corrected] in a consistent way. This causes the interpolation fraction xxratio to be outside the 0-1 range and sometimes produces crazy results. Because of the way that everything is signed, FPix R1P1, R1P2, and R2P1 are affected. The effect is on the RecHit errors in this panels.
This has been tested by @VinInn .