Fix cumulative bin accumulation and bin count bookkeeping in metrics.general.histogram

[Nicholas022400701]

PhysicsNeMo Pull Request

Description

closes #2005

_high_memory_bin_reduction_cdf in physicsnemo/metrics/general/histogram.py wrote the size of the current input into the last cumulative bin with = while every other bin, and the whole low memory twin _low_memory_bin_reduction_cdf, accumulate with +=. _count_bins always tries the high memory routine first, so whenever counts are accumulated across calls the last bin held only the last input. The public cdf function hits this as soon as it gets more than one input, because _compute_counts_cdf feeds the running counts back into _count_bins for each input. After cdf(x, y, bins=10) the last bin held len(y) while the other bins counted x and y together, and the normalization counts / counts[-1] produced values above one. With 10 samples in x and 5 in y the largest value was 2.8.

Histogram.update stored self.bin_edges.shape[0] as number_of_bins, which is the number of edges, while __init__ uses bins.shape[0] - 1. After an update that extends the bin range, number_of_bins was one larger than the number of rows in counts, and a later __call__ built the histogram with one bin more than before.

Changes:

• _high_memory_bin_reduction_cdf adds inputs.shape[0] to the last bin instead of assigning it, matching the low memory routine and the pdf routines.
• Histogram.update stores self.bin_edges.shape[0] - 1.
• New CPU test test/metrics/test_histogram_cdf.py checks that cdf(x, y) stays at or below one and matches a reference built from the concatenated samples, that the low and high memory routines agree when they add to a non zero existing counts for both the pdf and the cdf case, and that Histogram.update leaves number_of_bins, bin_edges and counts consistent.

Nothing else in the file changed. The existing test_histogram in test/metrics/test_metrics_general.py compares the low and high memory routines, but it passes the same counts tensor object to both calls and each routine mutates and returns that tensor, so it compares one tensor with itself. I left that test alone here and can tighten it in this PR if you want.

Verification on a CPU GitHub Actions runner with torch 2.14.0+cpu and Python 3.12:

• Fix branch, the 4 new cases plus the existing test_histogram and test_climate_efi pass, ruff format --check, ruff check and interrogate pass on the touched files, and the reproducer prints CDF_MAX 1.0 and NBINS 91 91 91: https://github.com/Nicholas022400701/oss-ci/actions/runs/35354064109
• Same test file dropped onto upstream main, 3 of the 4 cases fail and the reproducer prints CDF_MAX 2.799999952316284 and NBINS 92 91 91: https://github.com/Nicholas022400701/oss-ci/actions/runs/35354063983

I did not touch CHANGELOG.md in this PR. I can add a line under Fixed in the 2.3.0 section if you want it here.

AI disclosure: I used an AI coding agent to help write this patch, the tests and this description. I have read the change and the tests myself and I will answer review comments personally.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes. No public API or docstring changes, cdf now does what its docstring already says for several inputs.
• The CHANGELOG.md is up to date with these changes.
• An issue is linked to this pull request.
• If I am implementing a new model or modifying any existing model, I have followed the Models Implementation Coding Standards. Not applicable, no model is touched.

Dependencies

None.

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[github-actions]

CODEOWNERS review map

Current for commit 2f0886495064. An approval covers every file listed for that owner; one owner is sufficient for shared files.

⏳ @dallasfoster — 1 file(s)

• physicsnemo/metrics/general/histogram.py

⏳ @NickGeneva — 1 file(s)

• physicsnemo/metrics/general/histogram.py

No CODEOWNER

• test/metrics/test_histogram_cdf.py

Comment /codeowners-info to refresh.

[greptile-apps]

The PR appears safe to merge, with both fixes consistent with existing histogram contracts and covered by focused regression tests.

Summary

This PR fixes two histogram bookkeeping errors and adds focused regression coverage.

• Accumulates the final high-memory CDF bin across multiple inputs instead of replacing its prior count.
• Keeps Histogram.number_of_bins aligned with the number of count rows after bin-range updates.
• Tests multi-input CDF normalization, low/high-memory accumulation equivalence, and post-update shape consistency.

_{Reviews (1) · Last reviewed commit: "Add regression tests for cumulative hist..."}