Skip to content

Keep scores of differently configured metrics apart in get_metrics - #106

Merged
kPsarakis merged 4 commits into
masterfrom
fix/105-metric-name-collisions
Oct 30, 2026
Merged

kPsarakis merged 4 commits into
masterfrom
fix/105-metric-name-collisions

Conversation

@kPsarakis

@kPsarakis kPsarakis commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Fixes #105.

Stack: 1 of 3. This PR → #107 (tied scores in 1:1 selection) → #109 (metric input validation and docs).

Problem

Metric.name() returned only the class name, so get_metrics() stored F1Score(one_to_one=True) and F1Score(one_to_one=False) under the same key and one score overwrote the other. METRICS_ALL returned 6 of its 9 scores. Which variant survived depended on set iteration order: in one run "Precision" held the one_to_one=False value.

Change

  • Metric.name() appends every dataclass field that differs from its default, e.g. "F1Score(one_to_one=False)". Metrics built with default arguments keep their old names ("F1Score", "PrecisionTop10Percent"), so METRICS_CORE output, the README and the benchmark scripts are unchanged.
  • PrecisionTopNPercent keeps n in the name and appends only the other fields: "PrecisionTop70Percent(one_to_one=False)".
  • get_metrics() computes equal metric instances once, and raises ValueError when two different metrics report the same name (e.g. custom name() overrides), instead of silently overwriting.

I didn't use str(self) as suggested in the issue, because it would rename every key, including the defaults ("F1Score" → "F1Score(one_to_one=True)").

Breaking change

Code that reads a non-default metric by its bare class name now gets a different key: get_metrics(gt, {F1Score(one_to_one=False)}) returns "F1Score(one_to_one=False)" instead of "F1Score". This is recorded with a migration note in the changelog's Unreleased section, so it's better suited to a minor release than a patch.

Tests

  • Updated the 4 assertions that expected bare names for one_to_one=False variants.
  • Added tests for metric names, the issue's reproduction, METRICS_ALL returning every score, duplicates computed once, and the name-clash error.
  • Added tests that differently configured custom metrics (as in docs/metrics.md) get separate keys, and that instances differing only in a repr=False field raise.
  • Full suite: 311 passed locally.

CI fixes (separate commits)

Both are unrelated to #105, but this PR's CI needed them, and they sit at the bottom of the stack so #107 and #109 get them too.

  • macOS tests: every macOS job failed on TestJaccardEmbeddingIntegration with UserWarning: Skipping device Apple Paravirtual device that does not support Metal 2.0. torch emits it when transformers probes for MPS on GitHub's macOS runners, and filterwarnings = ["error"] turns it into a failure. It appeared since the 2026-10-14 master run, alongside torch 2.14.1 → 2.15.0 and a new macos-26-arm64 image, so master would fail the same way now. pyproject.toml ignores that one message.
  • Docs builds: all Documentation runs shared one pages concurrency group with cancel-in-progress, so a push to one PR cancelled the docs builds of the other open PRs. PR runs now get a group per branch; master pushes and manual runs, which deploy, still share one.

🤖 Generated with Claude Code

Metric.name() returned only the class name, so get_metrics() stored
F1Score(one_to_one=True) and F1Score(one_to_one=False) under the same
key and one score overwrote the other. METRICS_ALL returned 6 of its 9
scores, and which variant survived depended on set iteration order.

name() now appends every dataclass field that differs from its default,
e.g. "F1Score(one_to_one=False)". Instances with default arguments keep
their old names, so METRICS_CORE output is unchanged.
PrecisionTopNPercent keeps n in the name and appends only the other
fields.

get_metrics() computes equal metric instances once and raises
ValueError when two different metrics report the same name, instead of
silently overwriting a score.

Fixes #105

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.24%. Comparing base (f9d7234) to head (957414b).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   95.17%   95.24%   +0.06%     
==========================================
  Files          53       53              
  Lines        2820     2839      +19     
  Branches      440      446       +6     
==========================================
+ Hits         2684     2704      +20     
  Misses         83       83              
+ Partials       53       52       -1     
Flag Coverage Δ
unit 95.24% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
valentine/algorithms/matcher_results.py 98.12% <100.00%> (+0.03%) ⬆️
valentine/metrics/base_metric.py 96.77% <100.00%> (+3.02%) ⬆️
valentine/metrics/metrics.py 89.69% <100.00%> (+1.14%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

On GitHub's macOS runners, torch now warns "Skipping device Apple
Paravirtual device that does not support Metal 2.0" when transformers
probes for MPS. It appeared between the 2026-10-14 master run and now,
alongside torch 2.14.1 -> 2.15.0 and a new macos-26-arm64 runner image.
With filterwarnings = ["error"] it failed
TestJaccardEmbeddingIntegration on every macOS job. The warning is about
the runner's virtual GPU, so ignore that one message.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kPsarakis
kPsarakis force-pushed the fix/105-metric-name-collisions branch from b759eaa to c931ee6 Compare October 30, 2026 11:22
@kPsarakis
kPsarakis added this pull request to stack #108 October 30, 2026 11:29
kPsarakis and others added 2 commits October 30, 2026 12:29
The docs show users defining their own dataclass metrics. Check that
instances differing in a required field or a default_factory field get
separate keys in get_metrics(), and that instances differing only in a
repr=False field, which the name can't show, raise instead of
overwriting each other.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
All Documentation runs shared the "pages" concurrency group with
cancel-in-progress, so a push to one PR cancelled the docs build of
every other open PR. Give each PR its own group; pushes to master and
manual runs, which deploy, still share one group.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kPsarakis
kPsarakis merged commit c6a8000 into master Oct 30, 2026
31 checks passed
@kPsarakis
kPsarakis deleted the fix/105-metric-name-collisions branch October 30, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metrics Overwritten

1 participant