fix(f1): review follow-ups from #382#383
Merged
Merged
Conversation
Apply review feedback on the multiclass macro F1 fix from PR #382: - Add an early `n == 0` return so the `classes == 0` branch in the multiclass path is genuinely unreachable, then drop it. Without the guard, removing that branch would return NaN (0.0/0.0) on empty input. - Merge the two O(n) passes over y_true/y_pred (classes_set build and tp/support/predicted build) into a single pass. - Rename `sup` -> `support_count` for clarity. - Document the `to_f64_bits()` key convention (-0.0 vs +0.0), the positive-class bit-representation assumption baked into the binary path, and the `classes == 1` behaviour in the multiclass path. - Add `f1_single_class` test covering the single-class edge case. No behaviour change beyond #382; all existing tests pass unchanged.
Fixes `clippy::for_kv_map` lint in fastpair.rs:149 by iterating `distances.values()` instead of `distances.iter()` with an unused key binding.
3 tasks
Mec-iS
added a commit
that referenced
this pull request
Jul 21, 2026
…Recall/F1 (#384) * refactor(metrics): share per-class confusion counts across Precision/Recall/F1 Addresses the architectural review feedback from #382: the multiclass F1 path re-implemented per-class precision/recall bookkeeping using raw HashMaps, duplicating logic that already lived (in slightly different form) inside Precision and Recall. Introduce a pub(crate) `ConfusionCounts` helper (new src/metrics/confusion.rs) that computes the per-class tp / predicted / support / class set in a single pass. Precision and Recall each expose a crate-private `per_class_scores_from_counts` method deriving their per-class scores from those shared counts, and their `get_score` implementations are unified onto the same single-pass counts (the binary and multiclass paths now share one code path that either picks the positive class or macro-averages). F1's multiclass path builds the counts once and consumes the per-class precision/recall scores from Precision and Recall, eliminating the duplicated bookkeeping while keeping a single source of truth. Binary behaviour is preserved exactly (verified against all existing tests, including the {0,1} positive-class convention): for {0,1} labels the per-class positive-class precision/recall match the previous direct-computation formulas term-for-term. Also: add an `n == 0` early return to Precision/Recall (matching F1 post-#383) so the multiclass path can assume `classes >= 1`, and drop the now-dead `classes == 0` / `support.is_empty()` branches. README: bump the install snippet from ^0.4.3 to ^0.5.2 (current Cargo.toml version) and note the metrics refactoring in the roadmap. No behaviour change; all 432 lib tests + 67 doctests + 40 metrics tests pass under `cargo test --all-features`. `cargo fmt --check` and `cargo clippy --all-features -Dwarnings` are clean. * chore: bump to v0.5.3 - Cargo.toml: 0.5.2 -> 0.5.3 - README install snippet: ^0.5.2 -> ^0.5.3 - CHANGELOG: add [0.5.3] entry covering the metrics refactoring * refactor(metrics): address #384 review feedback - Rename ConfusionCounts::from -> new to avoid shadowing the std::convert::From trait convention (review blocker). - Add direct unit tests for ConfusionCounts (binary basic, multiclass, spurious predicted label, empty input, perfect predictions) covering tp/predicted/support/classes_set accessors — the helper was previously only tested indirectly through Precision/Recall/F1. - Add precision_binary_spurious_predicted_label test documenting the binary edge case where y_pred contains a label not in y_true: the new path uses predicted(positive) as the denominator (matching sklearn), so a spurious predicted label does not inflate it. The pre-refactor binary path would have counted it as a false positive (2/3 instead of 1.0). - Document that per_class_scores_from_counts iterates only over classes_set (y_true labels), so labels appearing only in y_pred are silently ignored — matching sklearn's behaviour. - Add order-independence comments for the HashMap::values().sum() calls in the Precision/Recall multiclass paths. - Clarify the binary Precision comment to note that the denominator is predicted(positive), not tp + fp_count, so spurious predicted labels do not affect the score. All 438 lib tests + 67 doctests + 46 metrics tests pass; fmt and clippy --all-features -Dwarnings clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #382 (no new issue; addresses reviewer feedback there).
Checklist
Current behaviour
F1::get_scorecorrectly computes the macro F-measure as the mean ofper-class F-beta scores (landed in #382), but the implementation had a
few rough edges flagged in code review:
if classes == 0 { 0.0 }branch in the multiclass path thatwas actually not dead — it is reachable via empty input, and
removing it without a guard would return
NaN(0.0 / 0.0).y_true/y_pred(one to buildclasses_set, another to buildtp_map/support/predicted).sup).to_f64_bits()HashMap key convention(
-0.0vs+0.0), the positive-class bit-representation assumptionbaked into the binary path, and the
classes == 1edge case.New expected behaviour
Same macro F1 behaviour as #382, with the review feedback applied:
n == 0return makesclasses == 0genuinely unreachable, sothe dead branch is removed safely (empty input still returns
0.0).sup->support_count.assumption, and the
classes == 1behaviour.f1_single_classtest covers the single-class edge case(perfect -> 1.0;
tp=2, pred=2, support=3->p=1.0, r=2/3-> 0.8).Change logs
classes == 0branch (after adding an
n == 0guard so it is truly unreachable),merged the two passes over the input into one, renamed
suptosupport_count, documented key/positive-class conventions, and added af1_single_classedge-case test. No behaviour change.Verification
cargo fmt --all -- --check— cleancargo clippy --all-features -- -Drust-2018-idioms -Dwarnings— cleancargo test --all-features— 432 lib tests + 67 doctests pass