Skip to content

Change FF LGAD resolution from 8ns to 30ps - #2938

Merged
simonge merged 4 commits into
mainfrom
Update-FF-TimeResolution
Sep 2, 2026
Merged

Change FF LGAD resolution from 8ns to 30ps#2938
simonge merged 4 commits into
mainfrom
Update-FF-TimeResolution

Conversation

@simonge

@simonge simonge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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

Updates the FF tracker timing resolution from 8ns which was the default for all detectors when introduced to the 30ps expected by the sensor (referenced from the now quite old digitization spreadsheet https://docs.google.com/spreadsheets/d/1s8oXj36SqIh7TJeHFH89gQ_ayU1_SVEpWQNkx6sETKs/edit?gid=238482234#gid=238482234)

Studies by @Garypenman highlighted looking for coincidences between the FF protons and FB electrons was being smeared out significantly more than anticipated. @ajentsch

A more detailed digitization approach and the correctness of the resolution value of other detectors are not being considered in this PR

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 #__)
  • 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 September 2, 2026 16:41
@github-actions github-actions Bot added the topic: tracking Relates to tracking reconstruction label Sep 2, 2026
@github-actions github-actions Bot added the topic: far-forward Far forward reconstruction label Sep 2, 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.

🟢 Approval recommended

The changes are a consistent, unit-explicit parameter update (30 * dd4hep::picosecond) in the relevant detector plugins and align with existing code expectations that timing resolutions are expressed in ns-equivalent DD4hep units.

Pull request overview

This PR updates the configured timing resolution for far-forward LGAD-based tracker subsystems from the long-standing default of 8 ns to 30 ps, aligning the digitization/reconstruction smearing with the expected sensor performance and reducing unintended coincidence-time smearing in downstream analyses.

Changes:

  • Set SiliconTrackerDigi_factory timeResolution to 30 * dd4hep::picosecond for RPOTS, FOFFMTRK, and B0TRK.
  • Set TrackerHitReconstruction_factory timeResolution to 30 * dd4hep::picosecond for the same detectors to propagate the improved timing to reconstructed hits.
File summaries
File Description
src/detectors/RPOTS/RPOTS.cc Updates Roman Pot digitization and rechit timing resolution to 30 ps.
src/detectors/FOFFMTRK/FOFFMTRK.cc Updates Forward Off-Momentum tracker digitization and rechit timing resolution to 30 ps.
src/detectors/B0TRK/B0TRK.cc Updates B0 tracker digitization and rechit timing resolution to 30 ps.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

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

@simonge
simonge requested a review from ajentsch September 2, 2026 19:54
Comment thread src/detectors/B0TRK/B0TRK.cc Outdated
@simonge

simonge commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

In any case, the change to 30ps is perfectly fine - it's around what we expect to deliver, we just have not been directly using any timing information up to this point, so updating the number is a good idea.

Thanks @ajentsch

Copilot AI review requested due to automatic review settings September 2, 2026 21:07

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.

🟡 Changes recommended

The new edm4eic::unit::ps expressions are likely double and are used to initialize a float config via aggregate initialization, which can cause a narrowing-conversion compile error unless cast to float.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread src/detectors/B0TRK/B0TRK.cc
Comment thread src/detectors/FOFFMTRK/FOFFMTRK.cc
Comment thread src/detectors/RPOTS/RPOTS.cc
@simonge
simonge enabled auto-merge September 2, 2026 22:08
@simonge
simonge added this pull request to the merge queue Sep 2, 2026
Merged via the queue into main with commit 1d279d4 Sep 2, 2026
159 of 162 checks passed
@simonge
simonge deleted the Update-FF-TimeResolution branch September 2, 2026 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: far-forward Far forward reconstruction topic: tracking Relates to tracking reconstruction

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants