Skip to content

Fix #3328: MinMaxMetric min/max values are silently polluted by Lig...#3354

Merged
bhimrazy merged 4 commits intoLightning-AI:masterfrom
JiwaniZakir:fix/3328-minmaxmetric-min-max-values-are-silentl
Apr 9, 2026
Merged

Fix #3328: MinMaxMetric min/max values are silently polluted by Lig...#3354
bhimrazy merged 4 commits intoLightning-AI:masterfrom
JiwaniZakir:fix/3328-minmaxmetric-min-max-values-are-silentl

Conversation

@JiwaniZakir
Copy link
Copy Markdown
Contributor

@JiwaniZakir JiwaniZakir commented Apr 4, 2026

Closes #3328

What does this PR do?

Fixes #3328

Registers min_val and max_val in MinMaxMetric via add_state() instead of assigning them as plain attributes. This ensures reset() properly restores both to their initialization bounds (inf and -inf respectively), fixing silent pollution from Lightning's sanity validation check.

Root cause: In src/torchmetrics/wrappers/minmax.py, min_val and max_val were set as plain torch.tensor attributes in __init__. Since Metric.reset() only resets states registered via add_state(), these values were never cleared on reset() — despite the docstring claiming otherwise. This meant values computed during Lightning's 2-batch sanity check persisted across the entire training run.

Fix: Replace the two plain attribute assignments (lines 79–80) with add_state() calls using dist_reduce_fx="min" and dist_reduce_fx="max" respectively. This also makes distributed reduction semantically correct for these values.

A new test test_reset_clears_min_max in tests/unittests/wrappers/test_minmax.py directly verifies that after calling reset(), min_val equals inf and max_val equals -inf, regardless of what was computed beforehand.

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃


This PR was created with AI assistance (Claude). The changes were reviewed by quality gates and a critic model before submission.


📚 Documentation preview 📚: https://torchmetrics--3354.org.readthedocs.build/en/3354/

Register min_val and max_val via add_state() so Metric.reset()
properly resets them to inf/-inf, preventing sanity-check
pollution (issue Lightning-AI#3328).
- Fix tensor equality assertions to use `.item()` for proper scalar comparison
- Add explicit type annotations to suppress Pylance false positive on `.compute()`
- Add `test_reset_no_pollution_across_epochs` to directly reproduce the Lightning
  sanity-check pollution scenario from issue Lightning-AI#3328
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 34%. Comparing base (eb03a45) to head (bb8fff8).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #3354    +/-   ##
=======================================
- Coverage      37%     34%    -3%     
=======================================
  Files         364     349    -15     
  Lines       20096   19901   -195     
=======================================
- Hits         7458    6696   -762     
- Misses      12638   13205   +567     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mergify mergify Bot added the ready label Apr 9, 2026
@bhimrazy bhimrazy enabled auto-merge (squash) April 9, 2026 14:51
@bhimrazy bhimrazy merged commit 6dd7dd4 into Lightning-AI:master Apr 9, 2026
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MinMaxMetric min/max values are silently polluted by Lightning's sanity validation check

4 participants