[Karnis052]

multiclass_metrics_fn only accepted the literal "hits@n" and always
returned HITS@1/5/10. It now also accepts "hits@K" for any positive
integer K and returns HITS@K. "hits@n" keeps its old behavior, since
existing code depends on it.

• Add "hits@K" for any positive integer K, validated with a clear
  ValueError for malformed cutoffs (non-integer, zero, negative).
• Share the true-class ranking computation between hits@ and mean_rank
  via _true_label_ranks, instead of recomputing argsort per metric.
• Document hits@K, hits@n, and mean_rank in the docstring and API docs.
• Add tests/core/test_multiclass_ranking_metrics.py and
  examples/multiclass_ranking_metrics.py.

Fixes #1248

[MateehUllah]

I found a correctness regression in _true_label_ranks().

np.argmax(argsort == np.asarray(y_true)[:, None], axis=1) returns 0 when a row contains no matching class, so the function incorrectly reports rank 1 for an out-of-range true label.

For example, y_true=np.array([2]) with y_prob=np.array([[0.8, 0.2]]) produces rank 1 and consequently HITS@1 == 1.0, although class 2 does not exist in the two-column prediction matrix.

The previous implementation would raise an IndexError instead of silently reporting a perfect rank.

Could we validate that each true label is within [0, y_prob.shape[1]), raise a clear ValueError for invalid labels, and add a regression test? This would prevent incorrectly inflated ranking metrics.

[github-actions]

Test Coverage

Total: 72.20% (19026 / 26350 lines)

Modified file	Coverage	Lines
pyhealth/metrics/multiclass.py	100.00%	116 / 116

Workflow run · 8e72059

[Karnis052]

Fixed in 857eeb7 — _true_label_ranks now validates y_true is within [0, num_classes) and raises a clear ValueError otherwise. Also added a regression test (test_out_of_range_label_raises) covering your exact example. Could you take another look?

[Logiquo]

/review

[github-actions]

Previous review

_{Reviewed commit c52f7c0}

[github-actions]

Previous review

AI Review

suggestion: need change
decision: need change

Thank you for this contribution. Configurable hits@​K in multiclass_metrics_fn is a useful addition. It keeps hits@​n and mean_rank behaving as before, validates malformed cutoffs clearly, and comes with docs, an example and tests. The logic looks correct, and I checked the test expectations by hand.

To get this merged:

• Raise coverage for pyhealth/metrics/multiclass.py. CI reports 52.94% for the file, against the guideline of at least 80% for each modified file. Whole-repo coverage is 71.8%, which meets the 60% bar. The new ranking code is exercised by your tests. The shortfall comes mostly from other metric branches in the file (ROC/PR, F1, calibration, prediction-set metrics) and the __main__ block, which have no tests. Please add a few tests that call multiclass_metrics_fn with those metrics, such as the default metrics, a calibration metric, and the prediction-set metrics with y_predset. That should lift the file past 80%. If you think the bar shouldn't apply here, say why in the PR and a maintainer can decide.

Optional suggestions:

• The description says argsort is no longer recomputed per metric. In the code, _true_label_ranks still runs once for each requested hits@ or mean_rank metric. You could compute the ranks once, lazily, and reuse them. Alternatively, reword the description.
• Add a trailing newline to the new files and remove the stray whitespace-only line in the test file.

_{Reviewed commit c52f7c0}

[solarsys]

/review

[github-actions]

_{Reviewed commit 8e72059}

[github-actions]

[low] Cutoff parser accepts non-ASCII digits and fails with an unclear error

str.isdigit() is true for characters such as '²', which int() then rejects with a raw 'invalid literal for int()' message. This bypasses the intended hits@​K validation message. Using cutoff.isascii() and cutoff.isdecimal() (or a regex such as [1-9][0-9]*) would make the check consistent with the error message. This is non-blocking.

[github-actions]

AI Review

suggestion: approve
decision: approve

Thank you for this contribution! It lets multiclass_metrics_fn accept "hits@​K" for any positive integer K and return HITS@​K. "hits@​n" and "mean_rank" keep their existing outputs. The change also computes the true-class ranking once and shares it between the ranking metrics, adds a clear ValueError for malformed cutoffs and out-of-range labels, and adds docs, an example and tests.

The change looks correct and ready to merge. Coverage is 100% for pyhealth/metrics/multiclass.py, which is above the 80% per-file bar. Whole-repo coverage is 72.2%, which is above the 60% bar. I read the code but could not run the tests.

Optional suggestions:

• str.isdigit() accepts Unicode digits such as "²". A metric like "hits@²" would then fail with a raw int() error instead of your friendly message. Using cutoff.isascii() and cutoff.isdigit() would fix that.
• Ties in probabilities are ranked by an unstable argsort. This was already the case before your change, so it is fine to leave as is.
• tests/core/test_multiclass_metrics.py is a new file of tests for the other existing metrics. That is useful, but it is not mentioned in the description.

_{Reviewed commit 8e72059}

[solarsys]

Approving. The ranks are computed once and shared, hits@n and mean_rank return the same values as before, and malformed cutoffs and out-of-range labels raise clear errors. I merged this branch onto current master, and both new test files pass. The non-ASCII-digit point from the AI review is fine to leave. Thanks, @Karnis052.