Splitattrs ncsave redo commonmeta #5538

pp-mo · 2023-10-10T13:07:57Z

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

pp-mo · 2023-10-10T14:32:32Z

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

This should be the last piece of the puzzle which completes https://github.com/orgs/SciTools/projects/5
- 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.

lib/iris/common/metadata.py

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 · 2023-10-12T09:12:58Z

lib/iris/tests/unit/common/metadata/test_CubeMetadata.py

+                "same": ("GaLb:GaLb", [True]),
+                "extra_global": ("GaL-:G-L-", [False, True]),
+                "extra_local": ("G-La:G-L-", [False, True]),
+                "same_global_local": ("GaL-:G-La", [False, True]),
+                "diff_global_local": ("GaL-:G-Lb", [False, True]),
+                "diffglobal_nolocal": ("GaL-:GbL-", [False]),
+                "diffglobal_samelocal": ("GaLc:GbLc", [False]),
+                "difflocal_noglobal": ("G-La:G-Lb", [False]),
+                "difflocal_sameglobal": ("GaLc:GaLd", [False]),
+                "diff_local_and_global": ("GaLc:GbLd", [False]),


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.

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

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

lib/iris/tests/unit/common/metadata/test_CubeMetadata.py

trexfeathers · 2023-10-12T13:21:11Z

lib/iris/tests/unit/common/metadata/test_CubeMetadata.py

+                "diffglobal_nolocal": ("GaL-:GbL-", [False]),
+                "diffglobal_samelocal": ("GaLc:GbLc", [False]),
+                "difflocal_noglobal": ("G-La:G-Lb", [False]),
+                "difflocal_sameglobal": ("GaLc:GaLd", [False]),


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.

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.

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 · 2023-11-08T14:37:32Z

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 · 2023-11-09T16:03:47Z

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.

…orator.

codecov · 2023-11-13T12:30:51Z

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 · 2023-11-14T13:27:17Z

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 · 2023-11-14T13:40:56Z

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.

lib/iris/common/_split_attribute_dicts.py

lib/iris/tests/unit/common/metadata/test_CubeMetadata.py

…ce test.

* Split-attrs: Cube metadata refactortests (#4993) * Convert Test___eq__ to pytest. * Convert Test_combine to pytest. * Convert Test_difference to pytest. * Review changes. * Split attrs - tests for status quo (#4960) * Tests for attribute handling in netcdf load/save. * Tidy test functions. * Fix import order exception. * Add cf-global attributes test. * Towards more pytest-y implemenation. * Replace 'create_testcase' with fixture which also handles temporary directory. * Much tidy; use fixtures to parametrise over multiple attributes. * Fix warnings; begin data-style attrs tests. * Tests for data-style attributes. * Simplify setup fixture + improve docstring. * No parallel test runner, to avoid error for Python>3.8. * Fixed for new-style netcdf module. * Small review changes. * Rename attributes set 'data-style' as 'local-style'. * Simplify use of fixtures; clarify docstrings/comments and improve argument names. * Clarify testing sections for different attribute 'styles'. * Re-enable parallel testing. * Sorted params to avoid parallel testing bug - pytest#432. * Rename test functions to make alpha-order match order in class. * Split netcdf load/save attribute testing into separate sourcefile. * Add tests for loaded cube attributes; refactor to share code between Load and Roundtrip tests. * Add tests for attribute saving. * Fix method names in comments. * Clarify source of Conventions attributes. * Explain the test numbering in TestRoundtrip/TestLoad. * Remove obsolete test helper method. * Fix small typo; Fix numbering of testcases in TestSave. * Implement split cube attributes. (#5040) * Implement split cube attributes. * Test fixes. * Modify examples for simpler metadata printouts. * Added tests, small behaviour fixes. * Simplify copy. * Fix doctests. * Skip doctests with non-replicable outputs (from use of sets). * Tidy test comments, and add extra test. * Tiny typo. * Remove redundant redefinition of Cube.attributes. * Add CubeAttrsDict in module __all__ + improve docs coverage. * Review changes - small test changes. * More review changes. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix CubeAttrsDict example docstrings. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Odd small fixes. * Improved docstrings and comments; fix doctests. * Don't sidestep netcdf4 thread-safety. * Publicise LimitedAttributeDict, so CubeAttrsDict can refer to it. * Fix various internal + external links. * Update lib/iris/cube.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Update lib/iris/cube.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Streamline docs. * Review changes. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Splitattrs ncload (#5384) * Distinguish local+global attributes in netcdf loads. * Small test fixes. * Small doctest fix. * Fix attribute load-save tests for new behaviour, and old-behaviour equivalence. * Split attrs docs (#5418) * Clarification in CubeAttrsDict examples. * CubeAttrsDict fix docstring typo. * Raise awareness of split attributes in user guide. * What's New entry. * Changes to metadata documentation. * Splitattrs ncsave redo (#5410) * Add docs and future switch, no function yet. * Typing enables code completion for Cube.attributes. * Make roundtrip checking more precise + improve some tests accordingly (cf. #5403). * Rework all tests to use common setup + results-checking code. * Saver supports split-attributes saving (no tests yet). * Tiny docs fix. * Explain test routines better. * Fix init of FUTURE object. * Remove spurious re-test of FUTURE.save_split_attrs. * Don't create Cube attrs of 'None' (n.b. but no effect as currently used). * Remove/repair refs to obsolete routines. * Check all warnings from save operations. * Remove TestSave test numbers. * More save cases: no match with missing, and different cube attribute types. * Run save/roundtrip tests both with+without split saves. * Fix. * Review changes. * Fix changed warning messages. * Move warnings checking from 'run' to 'check' phase. * Simplify and improve warnings checking code. * Fix wrong testcase. * Minor review changes. * Fix reverted code. * Use sets to simplify demoted-attributes code. * WIP * Working with iris 3.6.1, no errors TestSave or TestRoundtrip. * Interim save (incomplete?). * Different results form for split tests; working for roundtrip. * Check that all param lists are sorted. * Check matrix result-files compatibility; add test_save_matrix. * test_load_matrix added; two types of load result. * Finalise special-case attributes. * Small docs tweaks. * Add some more testcases, * Ensure valid sort-order for globals of possibly different types. * Initialise matrix results with legacy values from v3.6.1 -- all matching. * Add full current matrix results, i.e. snapshot current behaviours. * Review changes : rename some matrix testcases, for clarity. * Splitattrs ncsave redo commonmeta (#5538) * Define common-metadata operartions on split attribute dictionaries. * Tests for split-attributes handling in CubeMetadata operations. * Small tidy and clarify. * Common metadata ops support mixed split/unsplit attribute dicts. * Clarify with better naming, comments, docstrings. * Remove split-attrs handling to own sourcefile, and implement as a decorator. * Remove redundant tests duplicated by matrix testcases. * Newstyle split-attrs matrix testing, with fewer testcases. * Small improvements to comments + docstrings. * Fix logic for equals expectation; expand primary/secondary independence test. * Clarify result testing in metadata operations decorator. * Splitattrs equalise (#5586) * Add tests in advance for split-attributes handling cases. * Move dict conversion inside utility, for use elsewhere. * Add support for split-attributes to equalise_attributes. * Update lib/iris/util.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Update lib/iris/tests/unit/util/test_equalise_attributes.py Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Simplify and clarify equalise_attributes code. --------- Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * Fix merge-fail messaging for attribute mismatches. (#5590) * Extra CubeAttrsDict methods to emulate dictionary behaviours. (#5592) * Extra CubeAttrsDict methods to emulate dictionary behaviours. * Don't use staticmethod on fixture. * Add Iris warning categories to saver warnings. * Type equality fixes for new flake8. * Licence header fixes. * Splitattrs ncsave deprecation (#5595) * Small improvement to split-attrs whatsnew. * Emit deprecation warning when saving without split-attrs enabled. * Stop legacy-split-attribute warnings from upsetting delayed-saving tests. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com>

pp-mo added 5 commits October 10, 2023 13:21

Define common-metadata operartions on split attribute dictionaries.

d168a89

Tests for split-attributes handling in CubeMetadata operations.

c6c3d71

Small tidy and clarify.

0d9a4e4

Common metadata ops support mixed split/unsplit attribute dicts.

9d17da4

Clarify with better naming, comments, docstrings.

28ebcbb

pp-mo marked this pull request as ready for review October 10, 2023 14:28

pp-mo requested a review from trexfeathers October 10, 2023 14:28

pp-mo added the Type: Feature Branch Highlight this for a feature branch label Oct 10, 2023

pp-mo linked an issue Oct 10, 2023 that may be closed by this pull request

Common metadata operations on split-attributes #5444

Closed

trexfeathers requested changes Oct 11, 2023

View reviewed changes

lib/iris/common/metadata.py Show resolved Hide resolved

lib/iris/common/metadata.py Outdated Show resolved Hide resolved

lib/iris/common/metadata.py Outdated Show resolved Hide resolved

trexfeathers requested changes Oct 12, 2023

View reviewed changes

pp-mo added 2 commits November 13, 2023 11:57

Remove split-attrs handling to own sourcefile, and implement as a dec…

8d61d9b

…orator.

Remove redundant tests duplicated by matrix testcases.

fb52624

pp-mo added 2 commits November 14, 2023 02:23

Newstyle split-attrs matrix testing, with fewer testcases.

2cd51cd

Small improvements to comments + docstrings.

07ed579

pp-mo mentioned this pull request Nov 14, 2023

Splitattrs completion trial #5582

Closed

trexfeathers requested changes Nov 14, 2023

View reviewed changes

lib/iris/common/_split_attribute_dicts.py Show resolved Hide resolved

lib/iris/tests/unit/common/metadata/test_CubeMetadata.py Outdated Show resolved Hide resolved

pp-mo added 2 commits November 15, 2023 12:57

Fix logic for equals expectation; expand primary/secondary independen…

dca5dce

…ce test.

Clarify result testing in metadata operations decorator.

b00b37f

pp-mo force-pushed the splitattrs_ncsave_redo_commonmeta branch from dae3a28 to b00b37f Compare November 15, 2023 13:50

trexfeathers approved these changes Nov 15, 2023

View reviewed changes

trexfeathers merged commit 0c20608 into SciTools:FEATURE_split_attrs Nov 15, 2023
15 of 16 checks passed

pp-mo mentioned this pull request Nov 15, 2023

Common metadata operations on split-attributes #5444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitattrs ncsave redo commonmeta #5538

Splitattrs ncsave redo commonmeta #5538

pp-mo commented Oct 10, 2023 •

edited

Loading

pp-mo commented Oct 10, 2023 •

edited

Loading

trexfeathers left a comment

trexfeathers left a comment

trexfeathers Oct 12, 2023

pp-mo Nov 13, 2023

pp-mo Nov 13, 2023

trexfeathers Oct 12, 2023

pp-mo Nov 13, 2023 •

edited

Loading

pp-mo Nov 14, 2023

pp-mo commented Nov 8, 2023 •

edited

Loading

trexfeathers commented Nov 9, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

pp-mo commented Nov 14, 2023

pp-mo commented Nov 14, 2023 •

edited

Loading

trexfeathers left a comment

Splitattrs ncsave redo commonmeta #5538

Splitattrs ncsave redo commonmeta #5538

Conversation

pp-mo commented Oct 10, 2023 • edited Loading

pp-mo commented Oct 10, 2023 • edited Loading

trexfeathers left a comment

Choose a reason for hiding this comment

trexfeathers left a comment

Choose a reason for hiding this comment

trexfeathers Oct 12, 2023

Choose a reason for hiding this comment

pp-mo Nov 13, 2023

Choose a reason for hiding this comment

pp-mo Nov 13, 2023

Choose a reason for hiding this comment

trexfeathers Oct 12, 2023

Choose a reason for hiding this comment

pp-mo Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

pp-mo Nov 14, 2023

Choose a reason for hiding this comment

pp-mo commented Nov 8, 2023 • edited Loading

Additional note:

trexfeathers commented Nov 9, 2023

An update

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

pp-mo commented Nov 14, 2023

pp-mo commented Nov 14, 2023 • edited Loading

trexfeathers left a comment

Choose a reason for hiding this comment

pp-mo commented Oct 10, 2023 •

edited

Loading

pp-mo commented Oct 10, 2023 •

edited

Loading

pp-mo Nov 13, 2023 •

edited

Loading

pp-mo commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 13, 2023 •

edited

Loading

pp-mo commented Nov 14, 2023 •

edited

Loading