Combine cubes 2

[pp-mo]

Rework replaces #6331, addresses #6203

[codecov]

Codecov Report

Attention: Patch coverage is 85.26316% with 14 lines in your changes missing coverage. Please review.

Project coverage is 89.78%. Comparing base (8745465) to head (44bb2c6).
Report is 81 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/iris/util.py	59.09%	5 Missing and 4 partials ⚠️
lib/iris/_combine.py	90.56%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6334      +/-   ##
==========================================
- Coverage   89.80%   89.78%   -0.02%     
==========================================
  Files          90       90              
  Lines       23589    23632      +43     
  Branches     4402     4411       +9     
==========================================
+ Hits        21183    21217      +34     
- Misses       1662     1667       +5     
- Partials      744      748       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pp-mo]

Status update 2025-02-25

• functional code and APIs are basically complete and correct
• basic API documentation is now complete + correct
• doctests and examples need review
• all existing tests are passing
• most additional tests are now present
• some extra testing may be needed

The currently-failing doctests in the docsring of the combine_cubes() utility routine are just a placeholder.
(We do need a working example there...)

My current details to-do list

(maybe not exhaustive) ...

• add unit tests for
  • LoadPolicy.context()
  • equalise_cubes_kwargscontrol
    • what happens if this contains invalid settings -- does that need further handling ?
      (update: no I don't think this needs handling)
  • merge_unique control
  • CubeList combine methods
• review options + settings explanations for
  • `CombineOptions
    • especially "settings" are more directly appropriate for loading -- needs saying, at least
  • LoadPolicy
    • needs (at least) strong hint to refer to settings -- documented under CombineOptions. ?use example?
• add a proper example/doctests for
  • CombineOptions
  • LOAD_POLICY
  • combine_cubes
• consider using enums for switchable API options (at least the merge_concat_sequence values)
• cross-reference user documentation between (?all-of ?) ...
  • load functions
  • iris main module
  • LoadPolicy
  • user-guide loading chapter

[pp-mo]

I'm going to mark this "ready for review", since it is (I think) functionally complete.
But I must admit the list of to-dos is a bit daunting.

For a functional query, I'm still a bit unsure of the relationship complexity between CombineOptions and LoadPolicy,
not least because the default for the options of a combine_cubes call is defined by LOAD_POLICY.
And you can see that in the subsidiary _combine_options_asdict routine which I needed to pull out for sharing,which 'ought' to live in iris._combine, but actually relies on iris.loading .
Hmmm
(update : resolved -- see below}

Hopefully I can get a big chunk of the TODOs today.

[pp-mo]

Note:

Following considerable offline discussion with @trexfeathers @stephenworsley ,
We decided it was better to re-merge the definitions of CombineOptions and LoadPolicy
So I've now done that : f7ecf84

In short, LOAD_POLICY and LoadPolicy are now just aliases for COMBINE_POLICY (object) and CombineOptions (class).
The notable reasons were :

• since all APIs (internal included) took settings dictionaries and not settings objects, there was no need ever to create a CombineOptions object
  • ... and clearly it was over-complex to make a separate global control object for combine_cubes, so I was going to be using LOAD_POLICY to control that too
• in order to make either or both of CombineOptions and LoadPolicydocs legible, I was finding I had to replicate documentation writeup between the two
• the "settings" classes of a CombineObject wanted to be the same as those of a LoadPolicy, but these were determined by the needs of the loading modes -- and notably tje distinction between "default" and "legacy" which is meaningless for combine_cubes)

So in the end ... it makes more sense to have a single definition, parts of which are irrelevant to one use, than to separate the two for the sake of a small difference

[pp-mo]

Update 2025-02-26

I finally completed my checklist for adding tests 🍾

I've also added a whatsnew, and some really minimal redirection links from the iris main module API and the userguide loading chapter.
HOWEVER I have also realised that we are missing (probably need) a mention of combine in the UG chapter on merge+concatenate. I don't know quite how to contemplate that at this point.

For now, I'm just awaiting comments

[pp-mo]

Update : thanks @stephenworsley
I think I addressed all comments to-date
(which is not to say there is not more to come)

please re-review !

[stephenworsley]

I don't think there's a real point to answer for this one, as they aren't copies but different names for the actual same object.
Both references are set to the same thing here

ah yes, that's what I meant, I was just imprecise (or incorrect) in my language. I ask because I have seen some confusion in the past over how to expect configuration objects to behave when being modified or set (e.g. code of the form iris.COMBINE_POLICY = ... rather than iris.COMBINE_POLICY[key] = ..., which I imagine is not what we would expect to support). I think I recall that for some non-standard use cases, it mattered how these objects were imported so I wondered if there was any call for tests for what happens to the same object imported with different names. If there was any non-trivial interaction we wanted to ensure was supported, that might be worth considering a test for, but it may be the case as you say that everything is either trivial or ought to be unsupported.

[stephenworsley]

Looking good, Just a couple small notes from the tests.

[pp-mo]

I don't think there's a real point to answer for this one, as they aren't copies but different names for the actual same object.
Both references are set to the same thing here

ah yes, that's what I meant, I was just imprecise (or incorrect) in my language. I ask because I have seen some confusion in the past over how to expect configuration objects to behave when being modified or set (e.g. code of the form iris.COMBINE_POLICY = ... rather than iris.COMBINE_POLICY[key] = ..., which I imagine is not what we would expect to support). I think I recall that for some non-standard use cases, it mattered how these objects were imported so I wondered if there was any call for tests for what happens to the same object imported with different names. If there was any non-trivial interaction we wanted to ensure was supported, that might be worth considering a test for, but it may be the case as you say that everything is either trivial or ought to be unsupported.

OK I think I do see what you mean now.
I will add a warning about this to the docs where the control usage is explained.
See 857a4f5

[pp-mo]

Update 2024-03-6

Thanks @stephenworsley responded to comments so-far + up to date, I hope.
Please re-check !

Note that while other PRs are merging at present, unfortunately every merge tends to create conflict in the whastnew/latest.rst.

[stephenworsley]

Just a couple TODOs to remove.

[pp-mo]

OK I had to rebase yet again, but hopefully that addresses the stale TODOs you pointed out.

[stephenworsley]

Looks good, I think this is mergable now.

[pp-mo]

Many thanks @stephenworsley !