refactor: Some internal GSF refactorings II#4194
refactor: Some internal GSF refactorings II#4194benjaminhuth wants to merge 5 commits intoacts-project:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughChanged, the code has been. In the Changes
Sequence Diagram(s)sequenceDiagram
participant A as GsfActor (act)
participant T as TemporaryStates
participant S as Stepper
participant Surf as Surface
participant R as Range
participant P as ProjFunction
A->>T: Retrieve tips
T-->>A: Provide component tips
A->>A: Filter tips by weight cutoff
A->>S: Call updateStepper with Surf, R, and P
S-->>A: Return updated state
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Core/include/Acts/TrackFitting/detail/GsfActor.hpp (1)
491-524: 🛠️ Refactor suggestionVariable overshadowing and reweighting overhead, I sense.
- Within the loop at lines 499-500, overshadowed
cmpbecomes redefined also at line 508. Confusion, that can cause. Rename the loop variable to avoid confusion, you should:-for (const auto& cmp : range) { - const auto& [weight, pars, cov] = proj(cmp); +for (const auto& component : range) { + const auto& [weight, pars, cov] = proj(component);
- A second
TODOabout avoiding extra reweighting is noted at line 519. The code callsreweightComponentsat line 523, yes. Possibly unify the reweighting steps or ensure it is needed, you should.
🧹 Nitpick comments (2)
Core/include/Acts/TrackFitting/detail/GsfActor.hpp (2)
321-322: A trivial projector, this is.A pass-through lambda introduced at lines 321-322, it simply returns the same component with no transformation. Potentially you may omit the lambda or rename it for clarity, but needed for flexible usage in
updateStepper, it might be.
489-490: Not the stepper function's final resting place, hmmm.A
TODOto moveupdateStepperinto the stepper itself, declared here is. Good it may be to track this in an issue, so break the code later, you do not. Assist with that, I can.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/TrackFitting/detail/GsfActor.hpp(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
benjaminhuth
changed the title
|
1 participant
Pulled out of #2697 to disentangle changes
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit