-
Notifications
You must be signed in to change notification settings - Fork 2
Fix equal metrics logging #1161
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -378,6 +378,21 @@ pub fn sort_appraisal_outputs_by_investment_priority(outputs_for_opts: &mut Vec< | |||||||||||||
| }); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /// Counts the number of top appraisal outputs in a sorted slice that are indistinguishable | ||||||||||||||
| /// by both metric and fallback ordering. Excludes the first element from the count. | ||||||||||||||
| pub fn count_equal_and_best_appraisal_outputs(outputs: &[AppraisalOutput]) -> usize { | ||||||||||||||
|
||||||||||||||
| pub fn count_equal_and_best_appraisal_outputs(outputs: &[AppraisalOutput]) -> usize { | |
| pub(crate) fn count_equal_and_best_appraisal_outputs(outputs: &[AppraisalOutput]) -> usize { |
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 think this is overkill
Copilot
AI
Mar 3, 2026
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.
count_equal_and_best_appraisal_outputs will panic on an empty slice because it indexes outputs[0] / outputs[1..]. Since this is a pub function, it should defensively handle outputs.is_empty() (e.g., return 0 early) to avoid exposing a panic as part of the public API.
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 guess in practice, we won't call this function with an empty slice. I'd probably just handle this case in the function anyway, personally, but you could also just mention that it panics in the doc comment instead.
Copilot
AI
Mar 3, 2026
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.
There are extensive existing unit tests in this module, but the new tie-detection logic in count_equal_and_best_appraisal_outputs isn’t covered. Adding tests for cases like: empty/singleton slices, equal metric but fallback differentiates, and equal metric + fallback tie would help prevent regressions in the logging behavior.
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.
Some tests would be nice. Copilot would probably do an ok job!
Copilot
AI
Mar 4, 2026
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.
The new tests call count_equal_and_best_appraisal_outputs on outputs that haven’t been sorted with sort_appraisal_outputs_by_investment_priority, but the helper’s docstring explicitly requires a sorted slice. This makes the tests depend on the construction order rather than the intended contract; consider sorting outputs in these tests before asserting (same applies to the other new count_equal_best_* tests below).
| let outputs = | |
| appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); | |
| let mut outputs = | |
| appraisal_outputs_with_investment_priority_invariant_to_assets(metrics, &asset); | |
| sort_appraisal_outputs_by_investment_priority(&mut outputs); |
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.
The doc comment still says “Print debug message…”, but this helper is named
log_on_equal_appraisal_outputsand uses the logger. Updating the comment wording (and/or the comment at the call site that says “Warn”) would avoid confusion about log level/behavior.