Skip to content

Fix Cube.rolling_window with lazy auxiliary coordinates (#6480)#7123

Open
gaoflow wants to merge 2 commits into
SciTools:mainfrom
gaoflow:fix/rolling-window-lazy-aux-coord
Open

Fix Cube.rolling_window with lazy auxiliary coordinates (#6480)#7123
gaoflow wants to merge 2 commits into
SciTools:mainfrom
gaoflow:fix/rolling-window-lazy-aux-coord

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

🚀 Pull Request

Description

Closes #6480 (and the duplicate #6639).

:meth:iris.cube.Cube.rolling_window raised a TypeError when the cube had a lazy auxiliary coordinate running along the windowed dimension:

import numpy as np, dask.array as da
from iris.coords import DimCoord, AuxCoord
from iris.cube import Cube
from iris.analysis import MEAN

dimco = DimCoord(np.arange(12), standard_name="time", units="s")
auxco = AuxCoord(da.arange(12), long_name="x")   # lazy
cube = Cube(np.arange(12),
            dim_coords_and_dims=[(dimco, 0)],
            aux_coords_and_dims=[(auxco, 0)])
cube.rolling_window(coord="time", window=3, aggregator=MEAN)
TypeError: '>=' not supported between instances of 'tuple' and 'int'

Cause

When building the new coordinate bounds, rolling_window selects the first and last point of each window with:

new_bounds = new_bounds[:, (0, -1)]

A tuple index is interpreted by Dask as a multidimensional index (and so it tries to use 0 and -1 as indices into successive axes), which fails for a lazy array. NumPy happens to tolerate the tuple here, so the bug only surfaces for lazy coordinates.

Fix

Index with the list [0, -1] instead. NumPy and Dask both treat a list as selecting the first and last column, so the result is identical for real arrays and now also works (and stays lazy) for lazy ones. This is the fix hinted at by @pp-mo on the issue. Both the numeric and string-coordinate branches are updated.

Verification

  • New test Test_rolling_window::test_lazy_aux_coord — asserts a lazy aux-coord no longer fails, stays lazy (points and bounds), and equals the real-array result. It fails on main and passes here.
  • Manually confirmed identical results for lazy vs. real, numeric vs. string coordinates.
  • Full Test_rolling_window and tests/unit/util/test_rolling_window.py suites pass (25 tests).
  • ruff check / ruff format clean.

@gaoflow gaoflow force-pushed the fix/rolling-window-lazy-aux-coord branch from 81f6da8 to aa05a2e Compare June 18, 2026 21:42
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (746f0a6) to head (bc6142c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7123   +/-   ##
=======================================
  Coverage   90.15%   90.15%           
=======================================
  Files          91       91           
  Lines       24985    24985           
  Branches     4685     4685           
=======================================
  Hits        22526    22526           
  Misses       1682     1682           
  Partials      777      777           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ESadek-MO ESadek-MO self-requested a review June 25, 2026 12:52
@gaoflow gaoflow force-pushed the fix/rolling-window-lazy-aux-coord branch from aa05a2e to e4ea7a2 Compare June 25, 2026 15:30
@gaoflow

gaoflow commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and resolved the conflict from the towncrier changelog migration:

  • kept docs/src/whatsnew/latest.rst as the generated towncrier stub
  • moved this PR's whatsnew entry to changelog/7123.bugfix.rst

Local verification:

  • git diff --check
  • uvx ruff check lib/iris/cube.py lib/iris/tests/unit/cube/test_Cube.py
  • uvx ruff format --check lib/iris/cube.py lib/iris/tests/unit/cube/test_Cube.py

I could not run the targeted pytest locally because this environment is missing the Iris runtime dependency cf_units.

@ESadek-MO ESadek-MO left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gaoflow, thanks for this one!

Only a few small changes, mostly preference and consistence rather than any large issues!

def test_lazy_aux_coord(self):
# A lazy aux-coord paralleling the rolled dimension must not cause a
# failure, and should give the same result as a real one, staying lazy
# (see #6480).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# (see #6480).
# (see #6480).
window = 2

Not drastically important, but keeping related tests aligned helps with readability.

Comment thread lib/iris/tests/unit/cube/test_Cube.py Outdated
# failure, and should give the same result as a real one, staying lazy
# (see #6480).
self.cube.add_aux_coord(AuxCoord(da.arange(6), long_name="lazy_extra"), 0)
res_cube = self.cube.rolling_window("val", iris.analysis.MEAN, 3)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
res_cube = self.cube.rolling_window("val", iris.analysis.MEAN, 3)
res_cube = self.cube.rolling_window("val", iris.analysis.MEAN, window, mdtol=0)

Comment thread docs/src/whatsnew/latest.rst Outdated
Comment thread lib/iris/cube.py Outdated
Comment on lines +5246 to +5248
# Index with a list rather than a tuple: a tuple is interpreted
# as a multidimensional index by Dask, so lazy coordinates would
# otherwise fail here (see #6480).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is needed at all, but if you'd like to keep it I'd shorten it.

Comment thread lib/iris/tests/unit/cube/test_Cube.py Outdated
Comment on lines +1090 to +1092
# A lazy aux-coord paralleling the rolled dimension must not cause a
# failure, and should give the same result as a real one, staying lazy
# (see #6480).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think this is needed, it's implicit in the test.

gaoflow added 2 commits June 26, 2026 13:43
rolling_window built each window's coordinate bounds with new_bounds[:,
(0, -1)]. A tuple index is treated by Dask as a multidimensional index,
so a lazy coordinate along the windowed dimension raised a TypeError.
Index with the list [0, -1] instead, which numpy and Dask both treat as
selecting the first and last column, giving an identical (and still lazy)
result.

Fixes SciTools#6480.
@gaoflow gaoflow force-pushed the fix/rolling-window-lazy-aux-coord branch from e4ea7a2 to bc6142c Compare June 26, 2026 11:43
@gaoflow

gaoflow commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Updated in bc6142cd and rebased onto current main.

Review follow-up:

  • shortened the Dask indexing comment in cube.py
  • aligned the regression test with the nearby window = 2 / mdtol=0 style
  • removed the extra explanatory test comment
  • kept the existing changelog/7123.bugfix.rst fragment

Verification after the update:

uv run --with pytest --with pytest-mock --with requests --with filelock --with distributed --with-editable . python -m pytest lib/iris/tests/unit/cube/test_Cube.py::Test_rolling_window::test_lazy_aux_coord -q
ruff check lib/iris/cube.py
ruff format --check lib/iris/cube.py lib/iris/tests/unit/cube/test_Cube.py
uv run --with towncrier towncrier build --draft --version 0.0
git diff --check origin/main...HEAD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Failing rolling window

2 participants