Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "combine cubes" function public. #82

Draft
wants to merge 5 commits into
base: load_refactor
Choose a base branch
from

Conversation

pp-mo
Copy link
Owner

@pp-mo pp-mo commented Oct 28, 2024

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.

@pp-mo pp-mo linked an issue Oct 28, 2024 that may be closed by this pull request
@pp-mo pp-mo force-pushed the public_combine_cubes branch from 9c7df0b to 8986330 Compare October 28, 2024 18:17
@pp-mo pp-mo marked this pull request as ready for review October 28, 2024 18:22
@@ -634,6 +634,67 @@ def concatenate(
check_derived_coords=check_derived_coords,
)

def combine(self, options=None, merge_require_unique=False, **kwargs):
"""Combine cubes according to :class:`iris~.io.loading.LoadPolicy` options.
Copy link

@stephenworsley stephenworsley Oct 30, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

@pp-mo pp-mo Oct 30, 2024

Choose a reason for hiding this comment

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

I wonder if there may be some confusion caused by the link between CubeList.combine and LoadPolicy ... 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_cubes in 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

@pp-mo pp-mo marked this pull request as draft October 30, 2024 11:54
@pp-mo
Copy link
Owner Author

pp-mo commented Oct 30, 2024

NOTE: converted to draft.
We really don't want to merge this onto load_refactor branch : need to get that into Iris/main first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make _combine_cubes public, expand API
2 participants