Skip to content

Disable computation of Q probability in SiPixel template CPE for iterative tracking #39776

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.

Sign up for GitHub

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

Merged
cmsbuild merged 5 commits into from
Oct 28, 2022
Merged

Disable computation of Q probability in SiPixel template CPE for iterative tracking #39776

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f6282af
Use SiPixel CPE (cluster repair) with speed=0; i.e., without ProbQ co…
mmasciov Oct 17, 2022
5aa1f4a
Remove warning when disabling probQ computation
mmasciov Oct 17, 2022
2bde976
update WithoutProbQ for phase2
slava77 Oct 19, 2022
8b4f5df
Restore previous warning, except for default value
mmasciov Oct 22, 2022
bd6dab7
Use float representation in comparison
mmasciov Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DataFormats/TrackerRecHit2D/interface/SiPixelRecHitQuality.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class SiPixelRecHitQuality {
if (prob > 1.0f && prob <= 1.0f + std::numeric_limits<float>::epsilon()) {
prob = 1;
} else if (prob < 0.0f || prob > 1.0f + std::numeric_limits<float>::epsilon()) {
warningOutOfBoundProb("Q", prob, qualWord);
//warningOutOfBoundProb("Q", prob, qualWord);
Copy link
Contributor

@mmusich mmusich Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per previous statement (cf #38967 (comment)) commented code is not advised in the reco stack.
If you are going to keep it, then at least put a comment on why it is so.

Copy link
Contributor Author

@mmasciov mmasciov Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's no objection, I can simply delete this line. I kept it commented out in case others think it may be useful to keep. If not, I will delete it.

Copy link
Contributor

@slava77 slava77 Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that the default value should not get a warning, but perhaps everything else still should.
@cms-sw/trk-dpg-l2 please advise

Copy link
Contributor

@slava77 slava77 Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guess that the default value should not get a warning, but perhaps everything else still should. @cms-sw/trk-dpg-l2 please advise

I guess my tag went only to the DPG conveners
@ferencek @mroguljic
(please clarify also if there is a reco contact replacing Oz)

Copy link
Contributor

@ferencek ferencek Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've been looking (unsuccessfully) for a reco contact for more than a year now.

Regarding the proposed code changes, simply commenting out or deleting a call to warningOutOfBoundProb does not look like a good solution to me as it changes the legacy behavior of this code. Possible solutions:

  1. As Slava suggested, modify the code so that the default value does not cause a warning
  2. Add some sort of verbosity flag which would allow suppressing warnings when the Q probability is not computed.

Copy link
Contributor Author

@mmasciov mmasciov Oct 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferencek, this is addressed in 8b4f5df.
Only the default value does not cause the warning, which otherwise is still there and active.

prob = 0;
}
double draw = (prob <= 1E-5) ? 255 : -log((double)prob) * probY_1_over_log_units;
Expand Down
5 changes: 4 additions & 1 deletion RecoLocalTracker/SiPixelRecHits/python/PixelCPEClusterRepair_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,7 @@

from RecoLocalTracker.SiPixelRecHits._templates2_default_cfi import _templates2_default
templates2 = _templates2_default.clone()

templates2_speed0 = _templates2_default.clone(
ComponentName = "PixelCPEClusterRepairWithoutProbQ",
speed = 0
)
1 change: 1 addition & 0 deletions RecoTracker/TrackProducer/python/TrackProducerIterativeDefault_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@
useSimpleMF = cms.bool(True),
SimpleMagneticField = cms.string("ParabolicMf"),
Propagator = cms.string('PropagatorWithMaterialParabolicMf'),
TTRHBuilder = cms.string('WithAngleAndTemplateWithoutProbQ')
)
3 changes: 3 additions & 0 deletions RecoTracker/TransientTrackingRecHit/python/TTRHBuilderWithTemplate_cfi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
Matcher = 'StandardMatcher',
ComputeCoarseLocalPositionFromDisk = False)

TTRHBuilderAngleAndTemplateWithoutProbQ = TTRHBuilderAngleAndTemplate.clone(ComponentName = 'WithAngleAndTemplateWithoutProbQ')

from Configuration.Eras.Modifier_trackingPhase2PU140_cff import trackingPhase2PU140
trackingPhase2PU140.toModify(TTRHBuilderAngleAndTemplate,
Copy link
Contributor

@slava77 slava77 Oct 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the failures in tests being all in phase-2 wfs
I guess a trackingPhase2PU140.toModify(TTRHBuilderAngleAndTemplateWithoutProbQ, is missing here ... and the PixelCPEGeneric.toModify (despite of it kind of being obviously without ProbQ).

Copy link
Contributor

@mmusich mmusich Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the PixelCPEGeneric.toModify (despite of it kind of being obviously without ProbQ)

The point of that modifier is to replace template reco with generic, when the former is not available (e.g. certain phase2 pixel sensor technologies) , so if TTRHBuilderAngleAndTemplateWithoutProbQ becomes the CPE of choice also for phase2, the modifier should replace that one too (as you correctly did in 2bde976)

Phase2StripCPE = 'Phase2StripCPE',
Expand All @@ -16,6 +18,7 @@
# uncomment these two lines to turn on Cluster Repair CPE
from Configuration.Eras.Modifier_phase1Pixel_cff import phase1Pixel
phase1Pixel.toModify(TTRHBuilderAngleAndTemplate, PixelCPE = 'PixelCPEClusterRepair')
phase1Pixel.toModify(TTRHBuilderAngleAndTemplateWithoutProbQ, PixelCPE = 'PixelCPEClusterRepairWithoutProbQ')

# Turn off template reco for phase 2 (when not supported)
from Configuration.ProcessModifiers.PixelCPEGeneric_cff import PixelCPEGeneric
Expand Down