Splitattrs ncsave redo commonmeta

[pp-mo]

Closes #5444

Specific handling of common-metadata operations on 'split' attribute dictionaries
(i.e. for CubeMetadata)

---

Consult Iris pull request check list

[pp-mo]

Ping @trexfeathers
I hope this is comprehensible, following some further efforts !

This should be the last piece of the puzzle which completes Split cube-attrs project
- except resolving the merge-back, cf #5152

[trexfeathers]

I still need to review the tests but I wanted to give you something to start on.

[trexfeathers]

Here's the rest of the review.

I ran test_CubeMetadata.py against the code in FEATURE_split_attrs and got 45 failures, so this clearly demonstrates we were missing something and you have now fixed it 👍

[trexfeathers]

I had to work out what the single vs double result was about. Could do with a comment explaining this is for the op_leniency parameter.

Some for combine and for difference.

[pp-mo]

I have completely now reviewed this, and am going to change it.

[pp-mo]

Still looking into this.
See comment below about plans to reorganise the "matrix" testing

[trexfeathers]

How did you choose which circumstances to cover?

For instance there is nothing for same-global, one-local (with one local being absent).

I can't tell if these are oversights, or deliberate decisions based on the mechanics.

[pp-mo]

You're right, but it's triggered me to review all of this.
I'm now confident that more cases are needed, but also there are more factors I haven't accounted for.

Having prototyped a more comprehensive approach to that, I came up with a full matrix with 1152 tests (!!)
So I realised that I need to provide AFAP specific tests for each of the expected 'regularities' in behaviour, with more limited combinations of 'other' factors. The current plan for these :

• local and global values (for same attribute) should be calculated independently of each other
• calculations on a given attribute(name) should be calculated exactly the same way for both local and global settings
• each result for "left OP right" should be identical to "right OP left"

However, the basic calculation (combining a right + left value), must still be tested for each of the 3 operations, both strict+lenient, so there's still a fair number of tests.

I think I'm nearly on top of this now,
but I'm building a new testing mechanism, so don't fret about that code for now.

[pp-mo]

I have now removed the idea of "targetted" testcases with selected combinations of factors.
The new approach is , broadly : more matrixing, leverage the expected 'same' results over different factors, and test the "local+global independence" separately from the other factors, to keep the number of tests under control.

[pp-mo]

Additional note:

Spotted an outstanding bug here in the tests
(this is in the existing code, not changed here)

It seems clear that the second assignment to self.lvalues should really be to self.rvalues
That would also explain the final comment

"Result has entirely EMPTY attributes ... is this maybe a mistake of the existing implementation ?"

[trexfeathers]

An update

@pp-mo and I realised that containing the specialised split-dict behaviour in method overrides in CubeMetadata would address most of my discomfort by giving the code an appropriate home and removing the need for conditional behaviour in the parent method.

[codecov]

Codecov Report

Attention: 37 lines in your changes are missing coverage. Please review.

Comparison is base (0b54d58) 89.25% compared to head (dca5dce) 89.45%.
Report is 141 commits behind head on FEATURE_split_attrs.

❗ Current head dca5dce differs from pull request most recent head b00b37f. Consider uploading reports for the commit b00b37f to get more accurate results

Files	Patch %	Lines
lib/iris/fileformats/netcdf/saver.py	91.06%	12 Missing and 4 partials ⚠️
lib/iris/fileformats/netcdf/_thread_safe_nc.py	79.48%	7 Missing and 1 partial ⚠️
lib/iris/__init__.py	70.00%	2 Missing and 1 partial ⚠️
lib/iris/analysis/__init__.py	96.80%	1 Missing and 2 partials ⚠️
lib/iris/fileformats/netcdf/_dask_locks.py	92.68%	2 Missing and 1 partial ⚠️
lib/iris/_concatenate.py	98.57%	0 Missing and 1 partial ⚠️
lib/iris/cube.py	99.10%	0 Missing and 1 partial ⚠️
lib/iris/fileformats/name_loaders.py	0.00%	1 Missing ⚠️
lib/iris/quickplot.py	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           FEATURE_split_attrs    #5538      +/-   ##
=======================================================
+ Coverage                89.25%   89.45%   +0.20%     
=======================================================
  Files                       88       90       +2     
  Lines                    22197    22603     +406     
  Branches                  4858     5437     +579     
=======================================================
+ Hits                     19811    20220     +409     
+ Misses                    1641     1638       -3     
  Partials                   745      745              

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

[pp-mo]

OK @trexfeathers
thanks for your patience since, following the previous review, I re-wrote most of the new testing framework !

I have just re-scanned + tweaked the comments+docstrings, and I have now definitely stopped fiddling with this.
I think I've basically now addressed the previous review comments (though you may disagree).
I'll re-scan the above outstanding comments + flag the changes made, where appropriate.

[pp-mo]

Note: the recent linkcheck fail (e.g. on 07ed579) is fixed by #5556

[trexfeathers]

Very elegant, and I think it all makes sense to me. Having stepped through locally.

I only noticed a couple of things that need some attention.