CT-PPS Pixel local track reconstruction patch #21107
Conversation
…n RPixRoadFinder for negative charge hits
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
A new Pull Request was created by @fravera for master. It involves the following packages: RecoCTPPS/PixelLocal @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
| @@ -64,11 +64,11 @@ void RPixDetClusterizer::buildClusters(unsigned int detId, const std::vector<CTP | |||
| if(!is_in){ | |||
| //calibrate digi and store the new ones | |||
| electrons = calibrate(detId,adc,row,column,pcalibrations); | |||
| if(electrons < SeedADCThreshold_*ElectronADCGain_) electrons = SeedADCThreshold_*ElectronADCGain_; | |||
There was a problem hiding this comment.
The second problem is something similar to what already discussed for the central pixel code last week between Danek and Andrea.
this is not exactly the same.
There we have an override to calibrated digis loaded per pixel, but that override is still below the seed threshold.
Here, apparently there is no distinction between a seed and a regular pixel thresholds.
There was a problem hiding this comment.
It may also make sense to rename the threshold to seedADCLowerBound_ because it's not serving as a threshold anymore.
BTW, I noticed that in the original review we missed the CMS style violations for the variable names, which should be starting with lower case.
This cleanup could be made in a follow up PR.
|
type bugfix |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1
|
|
@fravera |
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 (and backports should be raised in the release meeting by the corresponding L2) |
|
merge |
we have just discovered a couple of issues in the reco code of the CTPPS pixels that is affecting our track efficiency. We have also found the reasons and prepared fixes.
It would be important if the fixes could already be put inside 940 and used for the re-reco… We know that the deadline is passed but if we could find a way to put these patches it would really be great in terms of our performances.
More in details, the modifications proposed are “minimal” and are shown in this comparison:
master...CTPPS:ctpps_pixeLocalTracks_patch1
The first bug is in RecoCTPPS/PixelLocal/src/RPixRoadFinder.cc : we observed it when reconstructing the tracks that use only 3 planes out of the 6.
The problem is at line 90 and 113 where the number of points found in the pattern is request to be greater than minRoadSize_ (currently set to 3)
instead of requesting greater or equal.
The second problem is something similar to what already discussed for the central pixel code last week between Danek and Andrea.
We have some pixels which show very low ADC values due to the non-uniform irradiation we are facing in CT-PPS.
Once calibrated, these pixels will show a negative charge and would be rejected.
Since the problem appears in the pixels where we have the maximum number of physics hits (and hence are also the most irradiated)
we would lose a large number of good tracks.
For this reason we included a small change in RecoCTPPS/PixelLocal/src/RPixDetClusterizer.cc
in order to save a fix charge in case the calibrated charge is below the threshold set from the python (variable already present in the merged version).
Please let us know what you think.