search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

Numerical stability fix #269

Closed aehart closed 5 months ago

aehart commented 5 months ago

PR description:

This PR fixes a numerical stability issue with the calculation of seed stub coordinates in the case of the triplet seeds. The issue is more fully described in the PR to central CMSSW, which has been merged and of which this PR is a backport: https://github.com/cms-sw/cmssw/pull/44471

tomalin commented 5 months ago

Adding Sara to this PR review, since she's familiar with the PurgeDuplicate::getInventedCoordsExtended() function that it changes.

tomalin commented 5 months ago

PurgeDuplicate::getInventedCoordsExtended() does one silly thing. It invents the coordinates, and then, if the conditions at https://github.com/cms-L1TK/cmssw/blob/stub_z_fix/L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc#L721 are met, it throws away the coordinates it has invented, and keeps the original ones. It would be more logical to check the condition first, and only invent the coordinates if they are to be used. (N.B. The outermost stub on the triplet is not used to calculate the r-z helix params, so does not lie on the helix, so can't be invented). Though I guess this doesn't explain the crash, since inventing the stub would then be stable in r-phi, where all 3 stubs lie on the helix.

aehart commented 5 months ago

PurgeDuplicate::getInventedCoordsExtended() does one silly thing. It invents the coordinates, and then, if the conditions at https://github.com/cms-L1TK/cmssw/blob/stub_z_fix/L1Trigger/TrackFindingTracklet/src/PurgeDuplicate.cc#L721 are met, it throws away the coordinates it has invented, and keeps the original ones. It would be more logical to check the condition first, and only invent the coordinates if they are to be used. (N.B. The outermost stub on the triplet is not used to calculate the r-z helix params, so does not lie on the helix, so can't be invented). Though I guess this doesn't explain the crash, since inventing the stub would then be stable in r-phi, where all 3 stubs lie on the helix.

I think you are right that this doesn't explain the crash that was seen. But it is silly, so I rearranged the code so this check is done first, and the actual calculations are only done if it fails. I also sprinkled in a few more consts.

Running over a few events, I see the exact same invented stub coordinates being returned by this function, as expected.