-
Notifications
You must be signed in to change notification settings - Fork 168
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
fix: Track selector printouts to be in line with actual cuts #3815
base: main
Are you sure you want to change the base?
Conversation
WalkthroughEnhancements to the Changes
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nitpicks
I suspect that there is an overload to print
which made this compile at the end. I think we should avoid such a function name.
Co-authored-by: Andreas Stefl <[email protected]>
Co-authored-by: Andreas Stefl <[email protected]>
Co-authored-by: Andreas Stefl <[email protected]>
Co-authored-by: Andreas Stefl <[email protected]>
Co-authored-by: Andreas Stefl <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Core/include/Acts/TrackFinding/TrackSelector.hpp (2)
307-318
: Hmmmm, well structured these lambda functions are!Clear separation of printing patterns, you have achieved. Improved readability, this brings to our codebase.
A small suggestion, I have. Add comments to explain when each lambda should be used:
- // for printing cuts set up with `within` + // For printing bounded cuts with both minimum and maximum values (e.g., "min <= value < max") - // for printing cuts set up with `checkMin` + // For printing lower-bounded cuts (e.g., "min <= value") - // for printing cuts set up with `checkMax` + // For printing upper-bounded cuts (e.g., "value <= max")
320-332
: Wise choice of printing functions for each cut type, this is!Match the actual cut checks in
isValidTrack
, these printouts now do. But a small suggestion for improvement, I have.Group the printouts to match the order in
isValidTrack
method, we should. Apply this change:printMinMax("loc0", cuts.loc0Min, cuts.loc0Max); printMinMax("loc1", cuts.loc1Min, cuts.loc1Max); - printMinMax("time", cuts.timeMin, cuts.timeMax); printMinMax("phi", cuts.phiMin, cuts.phiMax); printMinMax("eta", cuts.etaMin, cuts.etaMax); printMinMax("absEta", cuts.absEtaMin, cuts.absEtaMax); printMinMax("pt", cuts.ptMin, cuts.ptMax); + printMinMax("time", cuts.timeMin, cuts.timeMax); printMax("nHoles", cuts.maxHoles); printMax("nOutliers", cuts.maxOutliers); printMax("nHoles + nOutliers", cuts.maxHolesAndOutliers); printMax("nSharedHits", cuts.maxSharedHits); printMax("chi2", cuts.maxChi2); printMin("nMeasurements", cuts.minMeasurements);This order, it matches the validation sequence in
isValidTrack
method, hmm yes.
Minor fix to the printouts in the track selector to show the right limits applied to the counters.
Summary by CodeRabbit
New Features
etaMax
parameter and improved error handling for bin indexing.Bug Fixes