Make "combine cubes" function public. #82
Conversation
pp-mo
force-pushed
the
public_combine_cubes
branch
from
9c7df0b to
8986330
Compare
| ) | ||
|
|
||
| def combine(self, options=None, merge_require_unique=False, **kwargs): | ||
| """Combine cubes according to :class:`iris~.io.loading.LoadPolicy` options. |
There was a problem hiding this comment.
I wonder if there may be some confusion caused by the link between CubeList.combine and LoadPolicy as it may not be intuitive that changing the latter would affect the former. There may be a way of naming this method so that this connection is clearer. Something like "apply_load_policy" or "combine_like_load".
I suppose there is an idea of having separate objects controlling loading and combining (i.e. a CombinePolicy object), but I think that would probably overcomplicate things.
There was a problem hiding this comment.
I wonder if there may be some confusion caused by the link between
CubeList.combineandLoadPolicy... a way of naming this method ... like "apply_load_policy" or "combine_like_load".
As discussed today in @SciTools/peloton :
@pp-mo yes I do agree that this is a bit mixed up.
TBH I think it's a result of my hurrying to get LOAD_POLICY + friends into the Iris 3.11 release, while not including combine_cubes, or referring to it, because there was not time (!)
In retrospect, I wish the loading controls referred to combine_cubes directly as in a former draft. And since @stephenworsley suggested making 'combine_cubes' a public thing, making all this "one thing" would have been preferable!
So with some hindsight, I would now favour :
- rename LoadPolicy as CombinePolicy
- include the "merge_unique" keyword as an option (attribute) of CombinePolicy
- explicitly reference
combine_cubesin the docs for LOAD_POLICY
@trexfeathers pointed out, a desire to rename 'LoadPolicy' is not worth making an rc1 release for. But we can still do all the above, subsequent to 3.11, while leaving LoadPolicy in the top-level Iris API, as an (effective) alias of CombinePolicy.
Where from here ?
I'll set about making those changes here + we can see what it looks like
|
NOTE: converted to draft. |
WIP : we will want to wait on Iris#6199 before merging.
For now, I'm targetting that PR rather than Iris/main, so it's clear what the actual changes here are.