Skip to content

Fix cumulative argmax/argmin returning window-local indices (GH#11336)#11412

Open
C1-BA-B1-F3 wants to merge 1 commit into
pydata:mainfrom
C1-BA-B1-F3:fix-cumulative-argmax-11336
Open

Fix cumulative argmax/argmin returning window-local indices (GH#11336)#11412
C1-BA-B1-F3 wants to merge 1 commit into
pydata:mainfrom
C1-BA-B1-F3:fix-cumulative-argmax-11336

Conversation

@C1-BA-B1-F3

Copy link
Copy Markdown

Problem

When calling argmax() or argmin() on a cumulative rolling window, the result returns window-local indices (positions within the NaN-padded window) instead of absolute indices in the original array.

For example, with data [1, 2, 1.5, 3.5, 4]:

  • cumulative('time').argmax() returns [4, 4, 3, 4, 4]
  • Expected: [0, 1, 1, 3, 4]

Root Cause

Cumulative rolling constructs a window with NaN padding on the left. argmax/argmin returns the position within this padded window, not the absolute index in the original array.

For example, at position 2:

  • Window: [nan, nan, 1.0, 2.0, 1.5]
  • argmax returns 3 (position of 2.0 in the window)
  • But the absolute index should be 1 (position of 2.0 in the original array)

Fix

The fix detects cumulative rolling (window size == dimension size, not centered) and adjusts the returned indices by subtracting the NaN padding offset:

absolute = local - (window_size - 1 - position)

Regression Tests

  • test_cumulative_argminmax: Tests both argmax and argmin with 1D data
  • test_cumulative_argmax_2d: Tests argmax with 2D data

Closes #11336

When calling argmax/argmin on a cumulative rolling window, the result
returned window-local indices (positions within the NaN-padded window)
instead of absolute indices in the original array.

The fix detects cumulative rolling (window size == dimension size, not
centered) and adjusts the returned indices by subtracting the NaN
padding offset: absolute = local - (window_size - 1 - position).

Regression tests included for 1D and 2D cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cumulate+argmax uses padded index instead of absolute index

1 participant