-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor!: Rewrote the ambiguity solver for clarity and added Optional Hits Selector #3805
Conversation
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
@Ragansu do I understand that right that it is also computing and labeling hits as |
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.hpp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Core/include/Acts/AmbiguityResolution/ScoreBasedAmbiguityResolution.ipp
Outdated
Show resolved
Hide resolved
Co-authored-by: Carlo Varni <[email protected]>
Co-authored-by: Carlo Varni <[email protected]>
Yes and No, There is computing of number of tracks shared with a said hit, but its not assigning to I could rewrite the code to include this calculation into the ambi solver but I believe it should be part of the original const track object. So before ambiguity solver. |
@Ragansu can you fix the docs test? (assuming it is still there ...) |
You mean the one shown by sonar cube ? I probably can but it will take decent amount of time to make appropriate unit tests. |
ok we good to go on this one, failure is unrelated |
Quality Gate passedIssues Measures |
The Score based solver needed one more feature to be more compatible with Athena, that is optional hit selector. The optional selector enables users to add optional function to the hit selection stage which was previously NOT possible.
In order to do this I had to template the existing getCleanedOutTracks.
This gave me an opportunity to rewrite the whole solver again for efficiency and clarity.
Ragansu Chakkappai.
--- END COMMIT MESSAGE ---
@CarloVarni , @Corentin-Allaire