-
Notifications
You must be signed in to change notification settings - Fork 17
refactor of det order helper functions and tests #113
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
Conversation
noajshu
commented
Aug 29, 2025
- added better test for index-based ordering
- refactored the det order function so it uses smaller helpers and a switch statement
…e-visualizer-files Add visualization library to CMake build
…-and-fix-cmake-tests Fix CMake Python module path and add agent build instructions
…accept-bitstring Allow decode_to_errors to accept bitstring
Co-authored-by: Noureldin <noureldinyosri@gmail.com>
…date-tesseract Add explicit DetIndex branch for detector ordering
…ers-and-build_det_orders refactor detector order helpers
|
Also updated beam climbing behavior to do the max of (beam+1, num_det_orders) # trials |
LalehB
left a comment
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.
LGTM!
Thanks Noah, this is pretty cool!
| size_t cur = q.front(); | ||
| q.pop(); | ||
| auto neigh = graph[cur]; | ||
| std::shuffle(neigh.begin(), neigh.end(), rng); |
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.
I wonder why we are shuffling here? 🤔
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.
Ah, this is to generate diversity in the ensemble!
| return build_det_orders_index(dem, num_det_orders, rng); | ||
| } | ||
| throw std::invalid_argument("Unknown det order method"); | ||
| } |
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.
I wonder if we should have one of them as default 🤔