Skip to content

fix: expand track seeding default cfg into non-sqrt values - #2906

Open
wdconinc wants to merge 3 commits into
mainfrom
track-seeding-actual-values
Open

fix: expand track seeding default cfg into non-sqrt values#2906
wdconinc wants to merge 3 commits into
mainfrom
track-seeding-actual-values

Conversation

@wdconinc

Copy link
Copy Markdown
Contributor

Briefly, what does this PR introduce? Please link to any relevant presentations or discussions.

This PR updates the default error values used for track parameter estimation in the TrackSeedingConfig struct. The changes replace expressions involving square roots and unit conversions with their precomputed numerical values, making the code clearer. The numerical values are essentially identical (up to 4th digit rounding) to what was there before, just more easily interpretable. No major changes are expected by this PR.

Track parameter estimation error updates:

  • Replaced computed expressions for locaError, locbError, phiError, thetaError, qOverPError, and timeError with their evaluated numerical values in the TrackSeedingConfig struct. This change clarifies the default error settings and removes the need for runtime computation.

What is the urgency of this PR?

  • High (please describe reason below)
  • Medium
  • Low

What kind of change does this PR introduce?

  • Bug fix (issue: weird units removed)
  • New feature (issue #__)
  • Optimization (issue #__)
  • Updated parameters, constants (issue #__)
  • Updated documentation
  • other: __

Please check if any of the following apply

  • This PR requires changes to geometry (epic PR: __)
  • This PR requires changes to EDM4eic (EDM PR: __)
  • This PR introduces breaking changes. Please describe changes users need to make below.
  • This PR changes default behavior. Please describe changes below.
  • AI was used in preparing this PR. Please describe usage below.

Copilot AI lite review requested due to automatic review settings August 26, 2026 18:12
@github-actions github-actions Bot added the topic: tracking Relates to tracking reconstruction label Aug 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the default track-parameter estimation covariance values in TrackSeedingConfig by replacing std::sqrt(...)-based expressions and unit conversions with precomputed numeric constants, aiming to make the defaults easier to interpret.

Changes:

  • Replaced locaError, locbError, phiError, thetaError, qOverPError, and timeError default initializers with precomputed numeric values.
  • Simplified the time error default from a derived expression to a direct ns-scaled constant.
Suppressed comments (1)

src/algorithms/tracking/TrackSeedingConfig.h:205

  • timeError is now a magic number, and it's also rounded relative to the previous expression (sqrt(0.1 * mm / ns) * ns). To keep the change purely cosmetic/clarifying, consider using a more precise evaluated value and keep a brief derivation note inline.
  float timeError   = 5.475 * Acts::UnitConstants::ns;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/algorithms/tracking/TrackSeedingConfig.h
@wdconinc
wdconinc requested review from a team, ShujieL and bschmookler August 26, 2026 21:39
@wdconinc

Copy link
Copy Markdown
Contributor Author

FYI @ShujieL @bschmookler This doesn't apply any real changes (by design) but should make it clearer what values are actually used now.

@wdconinc
wdconinc requested a review from veprbl September 4, 2026 21:17
Comment thread src/algorithms/tracking/TrackSeedingConfig.h Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 21:21
@wdconinc
wdconinc force-pushed the track-seeding-actual-values branch from a554d0f to 8e4aa4c Compare September 4, 2026 21:22
@wdconinc
wdconinc removed the request for review from Copilot September 4, 2026 21:22
Comment thread src/algorithms/tracking/TrackSeedingConfig.h Outdated
Copilot AI review requested due to automatic review settings September 4, 2026 22:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new constants are rounded/re-expressed enough to slightly change the configured default covariances, conflicting with the stated intent of being essentially identical to the prior expressions.

Review details

Suppressed comments (2)

src/algorithms/tracking/TrackSeedingConfig.h:204

  • The new numeric defaults are rounded enough to slightly change the effective covariance (e.g., sqrt(1.5)=1.224744… vs 1.225). If the intent is to be numerically equivalent to the previous expressions, consider using the full evaluated constants (enough digits to round to the same float) rather than 3-decimal approximations.
  float locaError   = 1.225 * Acts::UnitConstants::mm;
  float locbError   = 1.225 * Acts::UnitConstants::mm;

src/algorithms/tracking/TrackSeedingConfig.h:208

  • qOverPError/timeError were previously defined via sqrt-based expressions; rewriting them as low-precision decimals (and qOverPError as an inverted number) reduces traceability and can change the value slightly. Using a higher-precision evaluated coefficient (and a direct "/ GeV" form) keeps the intent clear while still removing runtime sqrt.
  float phiError    = 0.1414 * Acts::UnitConstants::rad;
  float thetaError  = 0.0447 * Acts::UnitConstants::rad;
  float qOverPError = 1. / (6.325 * Acts::UnitConstants::GeV);
  float timeError   = 0.0183 * Acts::UnitConstants::ns;
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@wdconinc
wdconinc enabled auto-merge September 4, 2026 23:27
@eic eic deleted a comment from github-actions Bot Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: tracking Relates to tracking reconstruction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants