Fix for subspacing with cyclic cf.wi and cf.wo arguments#925
Fix for subspacing with cyclic cf.wi and cf.wo arguments#925davidhassell merged 2 commits intoNCAS-CMS:mainfrom
cf.wi and cf.wo arguments#925Conversation
| arg0, arg1 = value.value | ||
| n = ((arg0 - arg1) / period).ceil() | ||
| arg1 = arg1 + n * period |
There was a problem hiding this comment.
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:
cf-python/cf/dimensioncoordinate.py
Lines 363 to 384 in bb01e7c
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
sadielbartholomew
left a comment
There was a problem hiding this comment.
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.
Fixes #887