feat!: Allow trimming other non measurement track states #3993
Conversation
|
@pbutti discovered this after adding edge holes |
WalkthroughEnhancements to track state manipulation, yes! A new optional parameter Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
|
@pbutti it just didn't run because the PR is still on draft. |
![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)
Core/include/Acts/Utilities/TrackHelpers.hpp(5 hunks)
🔇 Additional comments (3)
Core/include/Acts/Utilities/TrackHelpers.hpp (3)
415-415: Documentation clarity, improved it has been.
Clear and consistent documentation for the new parameter across all three functions, hmm yes. Well-documented changes, these are.
Also applies to: 457-457, 499-499
418-418: Backward compatibility, maintained it is.
Wise choice, making trimOther optional with default value false. Breaking changes, this prevents. Consistent parameter ordering across all functions, observed it is.
Also applies to: 460-460, 502-502
439-441: Implementation consistency, achieved it has been.
Identical logic for handling trimOther in both front and back trimming functions, implemented it is. Clear and concise condition !typeFlags.test(TrackStateFlag::MeasurementFlag), used it is.
Also applies to: 481-483
acts-policybot
bot
dismissed
pbutti’s stale review
Invalidated by push of be47d9e
github-actions
bot
added
Component - Examples
andiwand
changed the title
andiwand
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Core/include/Acts/Utilities/TrackHelpers.hpp (1)
514-520: Default values, consider adding you should.To make the interface more forgiving and maintain backward compatibility, suggest adding default values for all boolean parameters, I do. Particularly useful for the new
trimOtherNoneMeasurementparameter, this would be.-void trimTrack(track_proxy_t track, bool trimHoles, bool trimOutliers, - bool trimMaterial, bool trimOtherNoneMeasurement) +void trimTrack(track_proxy_t track, bool trimHoles = false, + bool trimOutliers = false, bool trimMaterial = false, + bool trimOtherNoneMeasurement = false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Core/include/Acts/Utilities/TrackHelpers.hpp(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
🔇 Additional comments (3)
Core/include/Acts/Utilities/TrackHelpers.hpp (3)
415-415: Documentation, well written it is!Clear and consistent with existing style, the documentation for
trimOtherNoneMeasurementparameter is. Approve this change, I do.Also applies to: 463-463, 511-511
429-435: Wise implementation of non-measurement state detection, this is!Clear and efficient, the logic for detecting other non-measurement states is. Through the Force, we see:
- Proper exclusion of holes, outliers, and material states
- Correct use of flag testing
- Clean separation of concerns
517-520: Verify the usage patterns across the codebase, we must.Ensure consistent usage of the new parameter throughout the codebase, we should.
✅ Verification successful
Consistent and correct, the usage patterns are, hmmmm.
In all places where trim functions are called, proper they are. Same parameter pattern, all callers follow. No disturbance in the Force, I sense.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing calls to trim functions rg -A 2 "trim(Track(Front|Back)?)\(" --type cppLength of output: 2445
|
Adding new track state flags requires changing the trimming. Here I add an
othercase which should catch anything else which is not a measurement.Non breaking if we keep the default.
Summary by CodeRabbit
New Features
trimOtherNoneMeasurementto enhance track trimming functions, allowing for more granular control over track states.Bug Fixes
TrackFindingAlgorithmfor better performance and robustness.Documentation
Tests