Skip to content

Comments

Fix for subspacing with cyclic cf.wi and cf.wo arguments#925

Merged
davidhassell merged 2 commits intoNCAS-CMS:mainfrom
davidhassell:fix-cyclic-subspace-2
Feb 24, 2026
Merged

Fix for subspacing with cyclic cf.wi and cf.wo arguments#925
davidhassell merged 2 commits intoNCAS-CMS:mainfrom
davidhassell:fix-cyclic-subspace-2

Conversation

@davidhassell
Copy link
Collaborator

Fixes #887

@davidhassell davidhassell added this to the NEXTVERSION milestone Feb 23, 2026
@davidhassell davidhassell added the bug Something isn't working label Feb 23, 2026
Comment on lines +475 to +477
arg0, arg1 = value.value
n = ((arg0 - arg1) / period).ceil()
arg1 = arg1 + n * period
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).

Copy link
Member

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

All good, makes the fix and is sufficiently tested. My one comment of feedback is being raised as a separate Issue. Happy for you to merge when ready.

@davidhassell davidhassell merged commit 05d37d7 into NCAS-CMS:main Feb 24, 2026
1 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Field.subspace fails to work properly for cyclic field

2 participants