Fix rebinning of partially occupied histograms by johannes-mueller · Pull Request #235 · boschresearch/pylife

johannes-mueller · 2026-03-20T10:12:45Z

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

Copilot

Pull request overview

Fixes #234 by correcting how rebin_histogram(..., binning=int) determines target binning for partially occupied MultiIndex (e.g., sparse 2D histograms), avoiding over-fine bins when rebinned.

Changes:

Adjust rebin_histogram MultiIndex/int handling to compute a per-dimension “global” target IntervalIndex, then subset it per group to the occupied range.
Update _do_rebin_histogram NaN handling by dropping NaNs once up-front (instead of per-interval).
Add regression tests for fully/partially occupied 2D histograms (same/up/down) and document the fix in the changelog.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/pylife/utils/histogram.py`	Fixes bin calculation for sparse MultiIndex histograms when `binning` is an `int`; adjusts NaN filtering and a binning validity edge case.
`tests/utils/test_histogram.py`	Adds regression coverage for sparse vs fully occupied 2D histograms across same/up/down rebin scenarios.
`CHANGELOG.md`	Notes the bug fix under upcoming release bug fixes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T15:06:27Z

    return histogram.reorder_levels(original_names)

+def _with_range_index(hist, names_to_drop):
+    new_hist = hist.copy().reset_index(drop=False)


PEP 8 expects two blank lines between top-level function definitions. Please add an extra blank line before the new helper defs so formatting stays consistent with the rest of this module.

Copilot · 2026-03-24T15:06:28Z

+def _with_range_index(hist, names_to_drop):
+    new_hist = hist.copy().reset_index(drop=False)
+    for level in names_to_drop:


The helper parameter name names_to_drop is misleading because the levels are not dropped; they are encoded to integer codes and later restored. Consider renaming it to something like names_to_encode/names_to_map to make the intent clearer.

Suggested change

def _with_range_index(hist, names_to_drop):

new_hist = hist.copy().reset_index(drop=False)

for level in names_to_drop:

def _with_range_index(hist, names_to_encode):

new_hist = hist.copy().reset_index(drop=False)

for level in names_to_encode:

Copilot · 2026-03-24T15:06:28Z

+    assert rebinned.shape == (8,)
+
+
+def test_rebin_irregular_1d_histogam():


Typo in test name: histogam should be histogram for clarity and discoverability.

Copilot · 2026-03-24T15:06:28Z

+    pd.testing.assert_series_equal(rebinned, expected)
+
+
+def test_rebin_irregular_2d_histogam():


Typo in test name: histogam should be histogram for clarity and discoverability.

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

Work around pandas-dev/pandas#64825 Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T06:34:05Z

    def binning_is_overlapping_or_non_monotonic_increasing():
+        if binning.left.min() == binning.right.max():
+            return
        return (
-            len(binning) > 0
+            len(binning) > 1


In _fail_if_binning_invalid(), the early return when binning.left.min() == binning.right.max() skips validation even if binning contains multiple identical zero-length intervals. That allows overlapping/non-monotonic binnings to pass silently; consider removing this early return or restricting it to the single-bin case (e.g., only bypass validation when len(binning) <= 1).

Copilot · 2026-04-09T06:34:05Z

+def _with_range_index(hist, names_to_drop):
+    new_hist = hist.copy().reset_index(drop=False)
+    for level in names_to_drop:


_with_range_index() remaps index levels to integer codes but doesn’t actually drop them; the parameter name names_to_drop is misleading. Renaming it to something like levels_to_remap/levels_to_encode would make the intent clearer and reduce confusion for future maintainers.

Suggested change

def _with_range_index(hist, names_to_drop):

new_hist = hist.copy().reset_index(drop=False)

for level in names_to_drop:

def _with_range_index(hist, levels_to_remap=None, names_to_drop=None):

if names_to_drop is not None:

warnings.warn(

"'names_to_drop' is deprecated, use 'levels_to_remap' instead.",

DeprecationWarning,

stacklevel=2,

)

if levels_to_remap is None:

levels_to_remap = names_to_drop

if levels_to_remap is None:

levels_to_remap = []

new_hist = hist.copy().reset_index(drop=False)

for level in levels_to_remap:

Copilot · 2026-04-09T06:34:06Z

+    rebinned = hi.rebin_histogram(hist, 8, nan_default=True).dropna()
+
+    print(hist.shape, rebinned.shape)
+    assert rebinned.shape == (8,)


Avoid print() statements in tests; they add noise to test output and make failures harder to read. Please remove this debug print (the assertion already captures the expected behavior).

Copilot · 2026-04-09T06:34:06Z

+    assert rebinned.shape == (8,)
+
+
+def test_rebin_irregular_1d_histogam():


Test function name has a typo: histogam → histogram. Renaming improves discoverability and consistency with the rest of the test suite.

Copilot · 2026-04-09T06:34:06Z

+    pd.testing.assert_series_equal(rebinned, expected)
+
+
+def test_rebin_irregular_2d_histogam():


Test function name has a typo: histogam → histogram. Renaming improves discoverability and consistency with the rest of the test suite.

Copilot · 2026-04-09T06:34:07Z

+
+    print(rebinned)
+    print(expected)
+


Avoid print() statements in tests; they add noise to test output and make failures harder to read. Please remove these debug prints.

Copilot · 2026-04-09T06:34:07Z

+    print(rebinned)
+    print(expected)
+


Avoid print() statements in tests; they add noise to test output and make failures harder to read. Please remove these debug prints.

Copilot · 2026-04-09T06:34:07Z

+    print(rebinned)
+    print(expected)
+


Avoid print() statements in tests; they add noise to test output and make failures harder to read. Please remove these debug prints.

Copilot · 2026-04-09T06:34:07Z

+    print("###############")
+
+    print(hist)
+    print(expected)
+    print(rebinned)


Avoid print() statements in tests; they add noise to test output and make failures harder to read. Please remove these debug prints.

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

johannes-mueller temporarily deployed to release March 20, 2026 10:13 — with GitHub Actions Inactive

Fix rebinning of partially occupied histograms

a5613c1

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

johannes-mueller force-pushed the 234-histogram-rebinning branch from 24e15da to a5613c1 Compare March 20, 2026 10:18

johannes-mueller temporarily deployed to release March 20, 2026 10:18 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release March 20, 2026 10:23 — with GitHub Actions Inactive

johannes-mueller requested a review from Copilot March 20, 2026 13:42

Copilot started reviewing on behalf of johannes-mueller March 20, 2026 13:42 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

Comment thread src/pylife/utils/histogram.py Outdated

Comment thread tests/utils/test_histogram.py

johannes-mueller temporarily deployed to release March 24, 2026 13:23 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release March 24, 2026 13:28 — with GitHub Actions Inactive

johannes-mueller requested a review from Copilot March 24, 2026 15:01

Copilot started reviewing on behalf of johannes-mueller March 24, 2026 15:01 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

johannes-mueller added 2 commits March 24, 2026 16:12

Fix histogram rebinning for overlapping histograms

309eb17

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

Workaround pandas bug for histogram rebinning

e4ad206

Work around pandas-dev/pandas#64825 Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

johannes-mueller force-pushed the 234-histogram-rebinning branch from 1568c69 to e4ad206 Compare March 24, 2026 15:12

johannes-mueller temporarily deployed to release March 24, 2026 15:13 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release March 24, 2026 15:18 — with GitHub Actions Inactive

Support rebinning histogram with one invariant axis

690b1a3

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

johannes-mueller temporarily deployed to release March 26, 2026 07:45 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release March 26, 2026 07:50 — with GitHub Actions Inactive

johannes-mueller requested a review from Copilot April 9, 2026 06:29

Copilot started reviewing on behalf of johannes-mueller April 9, 2026 06:29 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

Raise if histogram is to be rebinned to multiple zero length bins

3c2bad7

Signed-off-by: Johannes Mueller <johannes.mueller4@de.bosch.com>

johannes-mueller temporarily deployed to release April 9, 2026 06:51 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release April 9, 2026 06:52 — with GitHub Actions Inactive

johannes-mueller temporarily deployed to release April 9, 2026 06:56 — with GitHub Actions Inactive

johannes-mueller merged commit 83f2c4c into develop Apr 9, 2026
24 checks passed

johannes-mueller deleted the 234-histogram-rebinning branch April 9, 2026 07:08

		assert rebinned.shape == (8,)


		def test_rebin_irregular_1d_histogam():

		pd.testing.assert_series_equal(rebinned, expected)


		def test_rebin_irregular_2d_histogam():

-def _with_range_index(hist, names_to_drop):
-    new_hist = hist.copy().reset_index(drop=False)
-    for level in names_to_drop:
+def _with_range_index(hist, levels_to_remap=None, names_to_drop=None):
+    if names_to_drop is not None:
+        warnings.warn(
+            "'names_to_drop' is deprecated, use 'levels_to_remap' instead.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+        if levels_to_remap is None:
+            levels_to_remap = names_to_drop
+    if levels_to_remap is None:
+        levels_to_remap = []
+    new_hist = hist.copy().reset_index(drop=False)
+    for level in levels_to_remap:

Conversation

johannes-mueller commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants