fix: correctly removing absEtaMax from kwargs of TrackSelector #4114
Conversation
WalkthroughSignificant modifications, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant CKF as addCKFTracks
participant TS as trackSelectorDefaultKWArgs
Caller->>CKF: Call addCKFTracks(tslist)
CKF->>TS: Invoke trackSelectorDefaultKWArgs(c) for each c in tslist
TS-->>CKF: Return TrackSelector.Config
CKF-->>Caller: Return configured TrackSelector
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (1)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
pbalek
force-pushed
the
fix-def-kwargs-trk-selector
branch
from
6465bf1 to
be9f8d6
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Examples/Python/python/acts/examples/reconstruction.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
Examples/Python/python/acts/examples/reconstruction.py
143-143: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (2)
Examples/Python/python/acts/examples/reconstruction.py (2)
147-174: Good use of dictionary merging with the|operator I see. Hmm, wise choice this is!The implementation elegantly handles the overwriting of arguments using the dictionary merge operator introduced in Python 3.9. For any parameter that needs to be overridden, passed through
overwriteArgsit can be, without changing the entire function logic.
1438-1442: Fixed the issue withabsEtaMax, this change has. Wise, you are!When multiple track selector configurations present are,
absEtaMax=Noneis now set in theoverwriteArgsdictionary. This prevents theNonevalue from being passed toacts.TrackSelector.Config, as described in the PR objectives. The core of the bug fix, this is.
pbalek
force-pushed
the
fix-def-kwargs-trk-selector
branch
from
f9d32af to
4569d5d
Compare
|
Correctly remove absEtaMax from kwargs for TrackSelector.Config if there are multiple trackSelectorConfigs
--- END COMMIT MESSAGE ---
If there are multiple
trackSelectorConfigs, then variableabsEtaMaxshouldn't be passed toacts.TrackSelector.Config.With previous implementation, the value of
absEtaMaxwas overwritten toNonebut thisNonevalue was still passed toacts.TrackSelector.Config.Now, the values of
absEtaMaxare overwritten withNoneand later striped from the kwargs dictionary viatrackSelectorDefaultKWArgsandacts.examples.defaultKWArgs. This way, theabsEtaMaxare not passed toacts.TrackSelector.Configat all.Summary by CodeRabbit