Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@

# Excludes for *all* pre-commit hooks as listed below:
# (these are documentation files, either old ones or aut-generated ones)
exclude: docs\/3\..*\d\/|docs\/_downloads\/|docs\/.*\/tutorial\.py
exclude: |
(?x)^(
recipes-docs/|
docs/
)

repos:

Expand Down
10 changes: 10 additions & 0 deletions Changelog.rst
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
Version NEXTVERSION
--------------

**2026-??-??**

* Fix for subspacing with cyclic `cf.wi` and `cf.wo` arguments
(https://github.com/NCAS-CMS/cf-python/issues/887)

----

Version 3.19.0
--------------

Expand Down
48 changes: 39 additions & 9 deletions cf/mixin/fielddomain.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
bounds_combination_mode,
normalize_slice,
)
from ..query import Query, wi
from ..query import Query, wi, wo
from ..units import Units

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -245,8 +245,8 @@ def _indices(self, config, data_axes, ancillary_mask, kwargs):
tuples of domain axis identifier combinations, each
of which has of a `Data` object containing the
ancillary mask to apply to those domain axes
immediately after the subspace has been created
by the ``'indices'``. This dictionary will always be
immediately after the subspace has been created by
the ``'indices'``. This dictionary will always be
empty if the *ancillary_mask* parameter is False.

"""
Expand Down Expand Up @@ -456,6 +456,30 @@ def _indices(self, config, data_axes, ancillary_mask, kwargs):
if debug:
logger.debug(" 1-d CASE 2:") # pragma: no cover

arg0, arg1 = value.value
if arg0 > arg1:
# Query has swapped operands (i.e. arg0 >
# arg1) => Create a new equivalant Query
# that has arg0 < arg1, for a new
# arg1. E.g. for a period of 360,
# cf.wi(355, 5) is transformed to
# cf.wi(355, 365).
#
# This is done (effectively) by repeatedly
# adding the cyclic period to arg1 until
# it is greater than arg0, taking into
# account any units that have been set.
period = item.period()
value = value.copy()
value.set_condition_units(period.Units)
arg0, arg1 = value.value
n = ((arg0 - arg1) / period).ceil()
arg1 = arg1 + n * period
Comment on lines +475 to +477
Copy link
Member

Choose a reason for hiding this comment

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

This code block does the job and sensibly so, but I feel like it would be better to pull out at least the core period-based boundary wrangling as a separate method, perhaps internal but I think could easily be argued as a useful one for users, that would 'normalise' queries for a given period. I notice we, at least (just from a git grep period), use some quite similar logic in the dimensioncoordinate module:

if d.increasing:
# Adjust value so it's in the range [c[0], c[0]+period)
n = ((c[0] - value) / period).ceil()
value1 = value + n * period
shift = c.size - np.argmax((c - value1 >= 0).array)
d.roll(0, shift, inplace=True)
if cell:
d0 = d[0].upper_bounds
else:
d0 = d.get_data(_fill_value=False)[0]
n = ((value - d0) / period).ceil()
else:
# Adjust value so it's in the range (c[0]-period, c[0]]
n = ((c[0] - value) / period).floor()
value1 = value + n * period
shift = c.size - np.argmax((value1 - c >= 0).array)
d.roll(0, shift, inplace=True)
if cell:
d0 = d[0].upper_bounds
else:
d0 = d.get_data(_fill_value=False)[0]

and it already quite close to what the anchor method does, plus in general it sounds like the kind of thing we'd possibly need to do in various places, so better to have it self-contained and reusable and ideally reduce use of similar logic elsewhere by replacing with a call to the new method.

So I suggest a new mixin method normalise_range or normalise_query or similar (probably there's a better name to be determined). What do you think? (It could always be marked as follow-on work to consolidate, if you'd prefer in the interests of the backlog of work/reviewing we have to get through for the new release not to update this PR.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this idea, but I think the various use cases might be a subtly different to make putting it in here a "quick" job. Let's follow this up in a another issue, like you suggest. I'm wondering if a function in cf.functions would be a good way to go.

Copy link
Member

@sadielbartholomew sadielbartholomew Feb 24, 2026

Choose a reason for hiding this comment

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

Yes that's fair enough - indeed it might need some thinking in terms of the best way to consolidate things. Let's raise an Issue after we merge this (I can do it if that's easiest).

if value.operator == "wi":
value = wi(arg0, arg1)
else:
value = wo(arg0, arg1)

size = item.size
if item.increasing:
anchor = value.value[0]
Expand Down Expand Up @@ -2020,12 +2044,18 @@ def cyclic(

# Check for axes that are currently marked as non-cyclic,
# but are in fact cyclic.
if (
len(cyclic) < len(self.domain_axes(todict=True))
and self.autocyclic()
):
cyclic.update(self._cyclic)
self._cyclic = cyclic
#
# Note: We have to do a "dry run" on the 'autocyclic' call
# in the if test in order to prevent corrupting
# self._cyclic in the case that an axis tested by
# autocyclic is already marked as cylcic, but
# nonetheless autocyclic returns False (sounds
# niche, but this really happens!).
if len(cyclic) < len(
self.domain_axes(todict=True)
) and self.autocyclic(config={"dry_run": True}):
self.autocyclic()
cyclic = self._cyclic.copy()

return cyclic

Expand Down
4 changes: 3 additions & 1 deletion cf/mixin/propertiesdatabounds.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
)
from ..functions import equivalent as cf_equivalent
from ..functions import inspect as cf_inspect
from ..functions import parse_indices
from ..functions import (
parse_indices,
)
from ..functions import size as cf_size
from ..query import Query
from ..units import Units
Expand Down
23 changes: 23 additions & 0 deletions cf/test/test_Field.py
Original file line number Diff line number Diff line change
Expand Up @@ -1290,6 +1290,14 @@ def test_Field_indices(self):
a[..., [0, 1, 6, 7, 8]] = np.ma.masked
self.assertTrue(cf.functions._numpy_allclose(g.array, a), g.array)

# Cyclic cf.wi with swapped operands (increasing coords)
for q in (cf.wi(315, 45), cf.wi(-45, -675)):
indices = f.indices(grid_longitude=q)
g = f[indices]
self.assertEqual(g.shape, (1, 10, 3))
x = g.dimension_coordinate("X").array
self.assertTrue((x == [-40, 0, 40]).all())

# wi (decreasing)
f.flip("X", inplace=True)

Expand Down Expand Up @@ -1346,6 +1354,14 @@ def test_Field_indices(self):
(x == [0, 40, 80, 120, 160, 200, 240, 280, 320][::-1]).all()
)

# Cyclic cf.wi with swapped operands (decreasing coords)
for q in (cf.wi(315, 45), cf.wi(-45, -675)):
indices = f.indices(grid_longitude=q)
g = f[indices]
self.assertEqual(g.shape, (1, 10, 3))
x = g.dimension_coordinate("X").array
self.assertTrue((x == [40, 0, -40]).all())

# wo
f = f0.copy()

Expand Down Expand Up @@ -3055,6 +3071,13 @@ def test_Field_cyclic_iscyclic(self):
f2.cyclic("X", iscyclic=False)
self.assertTrue(f2.iscyclic("X"))

# In the case that autocyclic thinks the axis is not cyclic,
# check that calling iscylcic (which calls cyclic) doesn't
# change the cyclicity!
f2.dimension_coordinate("X").del_bounds()
self.assertTrue(f2.iscyclic("X"))
self.assertTrue(f2.iscyclic("X"))

def test_Field_is_discrete_axis(self):
"""Test the `is_discrete_axis` Field method."""
# No discrete axes
Expand Down
Loading