Added ACTS as track fitter in shipDigiReco.#1104
Open
webbjm wants to merge 11 commits into
Open
Conversation
olantwin
force-pushed
the
actsIntegration
branch
from
c24121c to
eb5f2af
Compare
Contributor
|
Any updates? |
Contributor
|
@webbjm Any news? |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces the GenFit-based tracking pipeline with an ACTS-based reconstruction across config, CLI, and reconstruction modules; adds ACTS tracking, seeding, vertexing, and SBT DOCA computation while updating smearing parameters and adding a ChangesACTS Tracking Pipeline Migration
Sequence Diagram(s)sequenceDiagram
participant Reco as Reconstruction (shipDigiReco)
participant Hits as StrawHits Buffer
participant Cand as Candidate Generator (patRec / Truth)
participant ACTS as ACTS Tracker
participant Vtx as Vertex Fitter
participant SBT as SBT DOCA Calcs
Reco->>Hits: collect smeared straw hits
Reco->>Cand: generate/receive candidates
Cand->>ACTS: provide seeds & hit lists
ACTS->>ACTS: build BoundTrackParameters & SourceLinks
ACTS->>ACTS: run Kalman fitter → fitted tracks
ACTS->>Vtx: send fitted tracks
Vtx->>Vtx: perform vertexing (if >=2 tracks)
ACTS->>SBT: provide track endpoints/parameters
SBT->>SBT: propagate to SBT planes & compute DOCA
SBT->>Reco: store veto associations and output branches
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@macro/ShipReco.py`:
- Line 156: The code currently force-sets global_variables.patRec to False which
overrides CLI selections; remove or change the hard-coded assignment so
global_variables.patRec is derived from the CLI/options value (e.g., use
options.patRec or do not assign at all) rather than always setting False; update
the assignment near where global_variables.patRec is set in ShipReco.py (line
with global_variables.patRec) to read the incoming option (options.patRec) or
omit the override so the reconstruction logic in shipDigiReco.py that checks for
"AR"/"Truth" and pattern recognition will respect the user-specified --patRec
flag.
In `@python/shipDigiReco.py`:
- Line 21: The import and call are inconsistent: the module defines run_tracking
but shipDigiReco imports runTracking and later calls it without passing self;
update the import to import run_tracking from strawReco (or import strawReco and
reference strawReco.run_tracking) and change the invocation to call run_tracking
with the instance/context and candidates (e.g., run_tracking(self, candidates)
or strawReco.run_tracking(self, candidates)) so the correct function name and
the required self argument are used.
- Around line 98-100: In ShipDigiReco.__init__, self.detector is created with
acts.examples.StrawtubeBuilder() but the next line calls trackingGeometry() on
an undefined name `detector`; change the call to use the instance
(self.detector.trackingGeometry()) so self.trackingGeometry is built from the
previously assigned self.detector instance (keep the
acts.examples.StrawtubeBuilder() assignment and then call
self.detector.trackingGeometry()).
- Around line 118-122: The reconstruct method calls actsTracks but never
captures its fit results, then calls calculate_sbt_doca with an undefined
output_tracks; modify reconstruct to store the ACTS fit output (e.g.,
output_tracks = self.actsTracks() or whatever actsTracks returns), then pass
that variable into calculate_sbt_doca(self, output_tracks, self.digiSBT.det),
and ensure the code path that fills VetoHitOnTrack uses that returned track list
(update VetoHitOnTrack assignment/usage to reference the captured output_tracks
rather than relying on a missing global variable).
- Around line 154-170: The code crashes and silently produces no candidates for
non-"AR" modes because of a typo (globa_variables -> global_variables) and
missing branches for the supported patRec modes; fix the typo where patRec is
checked, and add explicit handling for the other advertised modes ("FH" and
"TemplateMatching") inside the same if/elif chain (use the existing findTracks()
for "AR", iterate MCTrack for "Truth", and either call or implement
corresponding seeding functions such as findFHTracks() and templateMatchSeeds()
or raise a clear error if not implemented), ensuring candidates is populated
before calling runTracking(candidates) and that the checks reference
global_variables.patRec consistently.
In `@python/strawReco.py`:
- Around line 1-4: The module references undefined symbols logger and
global_variables (used in the "too few hits" branch and the vertexing path)
causing NameError; fix by importing or defining the module state used
later—e.g., add an explicit import for the module or objects that provide logger
and global_variables (or initialize them at top of strawReco.py) so that the
functions/methods that call logger.debug/error/info and access
global_variables[...] can run; ensure the imported/defined logger implements the
same methods used and global_variables contains the keys expected by the
vertexing and "too few hits" logic.
- Around line 7-18: The helper make_seed incorrectly references an undefined
target_surface and mixes coordinate frames: replace target_surface with the
passed surface and compute angles/momentum using the rotated momentum mom_rec
(not the original mom) so phi/theta and q/p are in the same reconstruction frame
as global_pos/local_pos; keep local_pos from surface.globalToLocal(geo_ctx,
global_pos, ...) and build params_vec from local_pos[0], local_pos[1],
mom_rec.Phi(), mom_rec.Theta(), charge / mom_rec.Mag(), 0, then return
acts.BoundTrackParameters(surface, params_vec, cov_matrix,
acts.ParticleHypothesis.muon).
- Around line 112-152: clear the per-event association state and compute DOCA in
the correct plane: at the top of calculate_sbt_doca call
self.vetoHitOnTrackArray.clear() to reset associations for the event, then move
the per-track initializations distMin = large_value and hitID = -1 inside the
loop over output_tracks (i.e. before iterating sbt_digis) so each track starts
fresh; when computing DOCA use the Y/Z components instead of X/Y by taking
extrapolated_pos[1:3] and hit_pos[1:3] (e.g. doca =
np.linalg.norm(extrapolated_pos[1:3] - hit_pos[1:3])); leave the push_back to
self.vetoHitOnTrackArray.push_back(ROOT.vetoHitOnTrack(hitID, distMin)) after
the sbt loop so each track records its best match (hitID may remain -1 if none
found).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f04adcd-03ac-4236-b4ce-5b4fe5d21bab
📒 Files selected for processing (5)
CHANGELOG.mdmacro/ShipReco.pypython/StrawTracker-digi-config.jsonpython/shipDigiReco.pypython/strawReco.py
Comment on lines
+154
to
+170
| if global_variables.patRec == "AR": # Or whatever flag you use | ||
| # PatRec Source | ||
| raw_candidates = self.findTracks() | ||
| for cand in raw_candidates: | ||
| # cand is already in our dictionary format from the previous step | ||
| candidates.append(cand) | ||
| elif globa_variables.patRec == "Truth": | ||
| # Truth Source | ||
| for trID, tr in enumerate(self.sTree.MCTrack): | ||
| indices = [i for i, h in enumerate(self.strawHits) if int(h[4]) == trID] | ||
| pos = ROOT.TVector3(tr.GetStartX(), tr.GetStartY(), tr.GetStartZ()) | ||
| mom = ROOT.TVector3(tr.GetPx(), tr.GetPy(), tr.GetPz()) | ||
| charge = self.PDG.GetParticle(tr.GetPdgCode()).Charge() / 3.0 | ||
|
|
||
| candidates.append({"pos": pos, "mom": mom, "indices": indices, "charge": charge}) | ||
|
|
||
| output_tracks = runTracking(candidates) |
There was a problem hiding this comment.
Handle every supported --patRec mode here.
Line 160 uses globa_variables, which crashes the non-"AR" path. Even after fixing that typo, this method only seeds ACTS for "AR"/"Truth" while macro/ShipReco.py still advertises "FH" and "TemplateMatching", so those modes fall through with no candidates.
Suggested fix
- if global_variables.patRec == "AR": # Or whatever flag you use
+ if global_variables.patRec in {"AR", "FH", "TemplateMatching"}:
# PatRec Source
raw_candidates = self.findTracks()
for cand in raw_candidates:
- # cand is already in our dictionary format from the previous step
candidates.append(cand)
- elif globa_variables.patRec == "Truth":
+ elif global_variables.patRec == "Truth":
# Truth Source
for trID, tr in enumerate(self.sTree.MCTrack):
indices = [i for i, h in enumerate(self.strawHits) if int(h[4]) == trID]
pos = ROOT.TVector3(tr.GetStartX(), tr.GetStartY(), tr.GetStartZ())
mom = ROOT.TVector3(tr.GetPx(), tr.GetPy(), tr.GetPz())
charge = self.PDG.GetParticle(tr.GetPdgCode()).Charge() / 3.0
candidates.append({"pos": pos, "mom": mom, "indices": indices, "charge": charge})
+ else:
+ raise ValueError(f"Unsupported patRec mode for ACTS tracking: {global_variables.patRec}")🧰 Tools
🪛 Ruff (0.15.12)
[error] 160-160: Undefined name globa_variables
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/shipDigiReco.py` around lines 154 - 170, The code crashes and silently
produces no candidates for non-"AR" modes because of a typo (globa_variables ->
global_variables) and missing branches for the supported patRec modes; fix the
typo where patRec is checked, and add explicit handling for the other advertised
modes ("FH" and "TemplateMatching") inside the same if/elif chain (use the
existing findTracks() for "AR", iterate MCTrack for "Truth", and either call or
implement corresponding seeding functions such as findFHTracks() and
templateMatchSeeds() or raise a clear error if not implemented), ensuring
candidates is populated before calling runTracking(candidates) and that the
checks reference global_variables.patRec consistently.
Contributor
|
@webbjm Any news on this PR or ACTS more generally? |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/shipDigiReco.py (1)
294-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the inverted charge convention.
The documented convention says
k_y34 > k_y12is positive charge, but Line 301 assigns-1. That seeds ACTS with the wrong q/p sign and can break opposite-charge vertex selection.Proposed fix
k_y34 = params.get("k_y34") if k_y12 is not None and k_y34 is not None: - charge = -1 if k_y34 > k_y12 else 1 + charge = 1 if k_y34 > k_y12 else -1 else: charge = 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/shipDigiReco.py` around lines 294 - 303, The charge sign assignment in the trackParams section is inverted relative to the documented convention. According to the comments, dk > 0 (meaning k_y34 > k_y12) should correspond to positive charge, but the conditional assignment on line 301 incorrectly assigns -1 when k_y34 > k_y12. Reverse the charge sign logic so that charge is set to 1 (positive) when k_y34 > k_y12 and -1 (negative) when k_y34 is less than or equal to k_y12, aligning with the documented physics convention and ensuring ACTS receives the correct q/p sign.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macro/ShipReco.py`:
- Line 148: The line using tempfile.mktemp() is vulnerable to TOCTOU attacks.
Replace tempfile.mktemp(suffix=".root") with a secure temporary file creation
method such as tempfile.NamedTemporaryFile(suffix=".root", delete=False) which
will safely generate a temporary file path and return its name to pass to
ROOT.FairRootFileSink().
In `@python/shipDigiReco.py`:
- Around line 134-159: The issue is that this loop populates self.strawHits with
valid smeared hits, but the raw digiHit IDs stored elsewhere cannot be directly
used as indices into strawHits because invalid/skipped hits cause an index
mismatch. Create a mapping dictionary that associates each digiHit ID
(sm["digiHit"]) with its actual index position in self.strawHits (by recording
the list length before each push_back operation), then store this mapping as an
instance variable so that findTracks() and runTracking() can convert digiHit IDs
to the correct strawHits indices instead of using raw digiHit values as direct
indices.
In `@python/strawReco.py`:
- Around line 39-61: The drift-side sign calculation in the loop that processes
strawHits uses MC-truth coordinates from iHit[6] and iHit[7] to determine the
vector to the hit (vec_to_hit_x and vec_to_hit_y), which biases the
reconstruction. Replace this MC-truth dependent approach with a
reconstruction-only ambiguity strategy such as fitting both possible drift signs
or resolving the ambiguity from residuals after an initial fit, ensuring that
the sign determination at index 16 is based only on reconstructed data rather
than true hit information.
- Around line 154-180: The issue is that when a track lacks a reference surface
in the enumerate loop over output_tracks, the code continues without appending a
result to veto_results, causing a misalignment between veto_results and
output_tracks. To fix this, append the sentinel tuple (-1, 99999) to
veto_results immediately before the continue statement in the condition that
checks `if not track.hasReferenceSurface()`, ensuring that every track processed
has a corresponding result in veto_results.
---
Outside diff comments:
In `@python/shipDigiReco.py`:
- Around line 294-303: The charge sign assignment in the trackParams section is
inverted relative to the documented convention. According to the comments, dk >
0 (meaning k_y34 > k_y12) should correspond to positive charge, but the
conditional assignment on line 301 incorrectly assigns -1 when k_y34 > k_y12.
Reverse the charge sign logic so that charge is set to 1 (positive) when k_y34 >
k_y12 and -1 (negative) when k_y34 is less than or equal to k_y12, aligning with
the documented physics convention and ensuring ACTS receives the correct q/p
sign.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecc3c541-e336-48f7-b939-58dbf0d514b0
📒 Files selected for processing (5)
CHANGELOG.mdmacro/ShipReco.pypython/StrawTracker-digi-config.jsonpython/shipDigiReco.pypython/strawReco.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/StrawTracker-digi-config.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/shipDigiReco.py (1)
294-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the inverted charge convention.
The documented convention says
k_y34 > k_y12is positive charge, but Line 301 assigns-1. That seeds ACTS with the wrong q/p sign and can break opposite-charge vertex selection.Proposed fix
k_y34 = params.get("k_y34") if k_y12 is not None and k_y34 is not None: - charge = -1 if k_y34 > k_y12 else 1 + charge = 1 if k_y34 > k_y12 else -1 else: charge = 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/shipDigiReco.py` around lines 294 - 303, The charge sign assignment in the trackParams section is inverted relative to the documented convention. According to the comments, dk > 0 (meaning k_y34 > k_y12) should correspond to positive charge, but the conditional assignment on line 301 incorrectly assigns -1 when k_y34 > k_y12. Reverse the charge sign logic so that charge is set to 1 (positive) when k_y34 > k_y12 and -1 (negative) when k_y34 is less than or equal to k_y12, aligning with the documented physics convention and ensuring ACTS receives the correct q/p sign.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@macro/ShipReco.py`:
- Line 148: The line using tempfile.mktemp() is vulnerable to TOCTOU attacks.
Replace tempfile.mktemp(suffix=".root") with a secure temporary file creation
method such as tempfile.NamedTemporaryFile(suffix=".root", delete=False) which
will safely generate a temporary file path and return its name to pass to
ROOT.FairRootFileSink().
In `@python/shipDigiReco.py`:
- Around line 134-159: The issue is that this loop populates self.strawHits with
valid smeared hits, but the raw digiHit IDs stored elsewhere cannot be directly
used as indices into strawHits because invalid/skipped hits cause an index
mismatch. Create a mapping dictionary that associates each digiHit ID
(sm["digiHit"]) with its actual index position in self.strawHits (by recording
the list length before each push_back operation), then store this mapping as an
instance variable so that findTracks() and runTracking() can convert digiHit IDs
to the correct strawHits indices instead of using raw digiHit values as direct
indices.
In `@python/strawReco.py`:
- Around line 39-61: The drift-side sign calculation in the loop that processes
strawHits uses MC-truth coordinates from iHit[6] and iHit[7] to determine the
vector to the hit (vec_to_hit_x and vec_to_hit_y), which biases the
reconstruction. Replace this MC-truth dependent approach with a
reconstruction-only ambiguity strategy such as fitting both possible drift signs
or resolving the ambiguity from residuals after an initial fit, ensuring that
the sign determination at index 16 is based only on reconstructed data rather
than true hit information.
- Around line 154-180: The issue is that when a track lacks a reference surface
in the enumerate loop over output_tracks, the code continues without appending a
result to veto_results, causing a misalignment between veto_results and
output_tracks. To fix this, append the sentinel tuple (-1, 99999) to
veto_results immediately before the continue statement in the condition that
checks `if not track.hasReferenceSurface()`, ensuring that every track processed
has a corresponding result in veto_results.
---
Outside diff comments:
In `@python/shipDigiReco.py`:
- Around line 294-303: The charge sign assignment in the trackParams section is
inverted relative to the documented convention. According to the comments, dk >
0 (meaning k_y34 > k_y12) should correspond to positive charge, but the
conditional assignment on line 301 incorrectly assigns -1 when k_y34 > k_y12.
Reverse the charge sign logic so that charge is set to 1 (positive) when k_y34 >
k_y12 and -1 (negative) when k_y34 is less than or equal to k_y12, aligning with
the documented physics convention and ensuring ACTS receives the correct q/p
sign.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecc3c541-e336-48f7-b939-58dbf0d514b0
📒 Files selected for processing (5)
CHANGELOG.mdmacro/ShipReco.pypython/StrawTracker-digi-config.jsonpython/shipDigiReco.pypython/strawReco.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- python/StrawTracker-digi-config.json
🛑 Comments failed to post (4)
macro/ShipReco.py (1)
148-148:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace insecure temporary filename generation.
tempfile.mktemp()is TOCTOU-prone (CWE-377). Use a securely created temp file path instead.Suggested fix
import tempfile -sink = ROOT.FairRootFileSink(tempfile.mktemp(suffix=".root")) +tmp = tempfile.NamedTemporaryFile(suffix=".root", delete=False) +tmp.close() +sink = ROOT.FairRootFileSink(tmp.name) run.SetSink(sink)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.tmp = tempfile.NamedTemporaryFile(suffix=".root", delete=False) tmp.close() sink = ROOT.FairRootFileSink(tmp.name)🧰 Tools
🪛 Ruff (0.15.17)
[error] 148-148: Use of insecure and deprecated function (
mktemp)(S306)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@macro/ShipReco.py` at line 148, The line using tempfile.mktemp() is vulnerable to TOCTOU attacks. Replace tempfile.mktemp(suffix=".root") with a secure temporary file creation method such as tempfile.NamedTemporaryFile(suffix=".root", delete=False) which will safely generate a temporary file path and return its name to pass to ROOT.FairRootFileSink().Source: Linters/SAST tools
python/shipDigiReco.py (1)
134-159:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKeep candidate indices in the compact
strawHitsindex space.
self.strawHitscontains only valid smeared hits, butfindTracks()re-smears and stores rawdigiHitIDs.runTracking()then indexesstrawHits[i], so any invalid/skipped digi shifts the index and can fit the wrong measurement or go out of range. Reuse the same smeared-hit list and mapdigiHit -> strawHits index.Proposed fix
- for sm in self.SmearedHits: + self._straw_hit_index_by_digi_hit = {} + for straw_idx, sm in enumerate(self.SmearedHits): station = self.strawtubes.det[sm["digiHit"]].GetStationNumber() view = self.strawtubes.det[sm["digiHit"]].GetViewNumber() layer = self.strawtubes.det[sm["digiHit"]].GetLayerNumber() @@ iHit = ROOT.std.vector("float")() iHit += [0, station, layer, view, straw, trID, x, y, z, time, deltaE, drift, xtop, ytop, xbot, ybot] self.strawHits.push_back(iHit) + self._straw_hit_index_by_digi_hit[sm["digiHit"]] = straw_idx @@ - listOfIndices[trID].append(sm["digiHit"]) + straw_idx = self._straw_hit_index_by_digi_hit.get(sm["digiHit"]) + if straw_idx is None: + continue + listOfIndices[trID].append(straw_idx)Also applies to: 234-238, 270-282, 326-328
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/shipDigiReco.py` around lines 134 - 159, The issue is that this loop populates self.strawHits with valid smeared hits, but the raw digiHit IDs stored elsewhere cannot be directly used as indices into strawHits because invalid/skipped hits cause an index mismatch. Create a mapping dictionary that associates each digiHit ID (sm["digiHit"]) with its actual index position in self.strawHits (by recording the list length before each push_back operation), then store this mapping as an instance variable so that findTracks() and runTracking() can convert digiHit IDs to the correct strawHits indices instead of using raw digiHit values as direct indices.python/strawReco.py (2)
39-61:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove the MC-truth dependency from drift-side assignment.
The sign is derived from
iHit[6]/iHit[7], which are the true straw-point coordinates populated upstream. That leaks MC truth into the default reconstruction and biases fitted tracks; use a reconstruction-only ambiguity strategy instead, e.g. fit both drift signs or resolve from residuals after an initial fit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/strawReco.py` around lines 39 - 61, The drift-side sign calculation in the loop that processes strawHits uses MC-truth coordinates from iHit[6] and iHit[7] to determine the vector to the hit (vec_to_hit_x and vec_to_hit_y), which biases the reconstruction. Replace this MC-truth dependent approach with a reconstruction-only ambiguity strategy such as fitting both possible drift signs or resolving the ambiguity from residuals after an initial fit, ensuring that the sign determination at index 16 is based only on reconstructed data rather than true hit information.
154-180:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn one SBT association per output track.
When a track lacks a reference surface, this function skips appending a veto result, so
VetoHitOnTrackno longer lines up withRecoTracksby position. Append the sentinel(-1, 99999)before continuing.Proposed fix
for t_idx, track in enumerate(output_tracks): + distMin = 99999 + hitID = -1 if not track.hasReferenceSurface(): + veto_results.append((hitID, distMin)) continue start_params = track.parametersObject - distMin = 99999 - hitID = -1 - for s_idx, sbt_hit in enumerate(sbt_digis):🧰 Tools
🪛 Ruff (0.15.17)
[warning] 154-154: Loop control variable
t_idxnot used within loop bodyRename unused
t_idxto_t_idx(B007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/strawReco.py` around lines 154 - 180, The issue is that when a track lacks a reference surface in the enumerate loop over output_tracks, the code continues without appending a result to veto_results, causing a misalignment between veto_results and output_tracks. To fix this, append the sentinel tuple (-1, 99999) to veto_results immediately before the continue statement in the condition that checks `if not track.hasReferenceSurface()`, ensuring that every track processed has a corresponding result in veto_results.
Contributor
|
For the CI, how much do we rely on the custom ACTS version? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaced Genfit with ACTS for track fitting in the default reconstruction chain "shipDigiReco"
Checklist
Summary by CodeRabbit
Release Notes
New Features
--patRecpattern-reconstruction mode, “Truth”, for MC truth-based seeding.Changed