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

Split-attrs merge message #5590

Merged

Conversation

pp-mo
Copy link
Member

@pp-mo pp-mo commented Nov 17, 2023

Closes #5585

During consideration of #5585 with @trexfeathers, we realised that although the way merge compares attributes is OK, the way it constructs a report of an attribute matching failure has some subtle problems in the "new world" of split attributes.
As stated in part of the testing code here,

        # Since Cube.attributes is now a "split-attributes" dictionary, it is now
        # possible to have "cube1.attributes != cube1.attributes", but also
        # "set(cube1.attributes.keys()) == set(cube2.attributes.keys())".
        # I.E. it is now necessary to specifically compare ".globals" and ".locals" to
        # see *what* differs between two attributes dictionaries.

This also affects the code where it works out which keys are different in existence ("cube.attributes keys differ: ") or value ("cube.attributes values differ for keys:") between two cubes whose attributes don't match.
Hence the changes here.

As so often, it was also useful to add some new tests to pin down existing behaviour,
When run against the prior feature-branch, behaviour looks like this :

$ pytest lib/iris/tests/test_merge_NEWSTUFF.py::TestCubeMerge__split_attributes__error_messages
================================================================================= test session starts ==================================================================================
platform linux -- Python 3.11.6, pytest-7.4.2, pluggy-1.3.0
rootdir: /net/home/h05/itpp/git/iris/iris_main
configfile: pyproject.toml
collected 6 items                                                                                                                                                                      

lib/iris/tests/test_merge_NEWSTUFF.py ..FF..                                                                                                                                     [100%]

======================================================================================= FAILURES =======================================================================================
______________________________________________ TestCubeMerge__split_attributes__error_messages.test_splitattrs_keys_local_global_mismatch ______________________________________________
iris.exceptions.MergeError: failed to merge into a single cube.
  cube.attributes keys differ: 'b'

During handling of the above exception, another exception occurred:

self = <iris.tests.test_merge_NEWSTUFF.TestCubeMerge__split_attributes__error_messages testMethod=test_splitattrs_keys_local_global_mismatch>

    def test_splitattrs_keys_local_global_mismatch(self):
        # Since Cube.attributes is now a "split-attributes" dictionary, it is now
        # possible to have "cube1.attributes != cube1.attributes", but also
        # "set(cube1.attributes.keys()) == set(cube2.attributes.keys())".
        # I.E. it is now necessary to specifically compare ".globals" and ".locals" to
        # see *what* differs between two attributes dictionaries.
>       self._check_merge_error(
            attrs_1=CubeAttrsDict(globals=dict(a=1), locals=dict(b=2)),
            attrs_2=CubeAttrsDict(locals=dict(a=2)),
            expected_message="cube.attributes keys differ: 'a', 'b'",
        )

lib/iris/tests/test_merge_NEWSTUFF.py:1179: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
lib/iris/tests/test_merge_NEWSTUFF.py:1139: in _check_merge_error
    with self.assertRaisesRegex(
E   AssertionError: "cube.attributes keys differ: 'a', 'b'" does not match "failed to merge into a single cube.
E     cube.attributes keys differ: 'b'"
________________________________________ TestCubeMerge__split_attributes__error_messages.test_splitattrs_keys_local_match_masks_global_mismatch ________________________________________
iris.exceptions.MergeError: failed to merge into a single cube.
  cube.attributes values differ for keys: 

During handling of the above exception, another exception occurred:

self = <iris.tests.test_merge_NEWSTUFF.TestCubeMerge__split_attributes__error_messages testMethod=test_splitattrs_keys_local_match_masks_global_mismatch>

    def test_splitattrs_keys_local_match_masks_global_mismatch(self):
>       self._check_merge_error(
            attrs_1=CubeAttrsDict(globals=dict(a=1), locals=dict(a=3)),
            attrs_2=CubeAttrsDict(globals=dict(a=2), locals=dict(a=3)),
            expected_message="cube.attributes values differ for keys: 'a'",
        )

lib/iris/tests/test_merge_NEWSTUFF.py:1186: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
lib/iris/tests/test_merge_NEWSTUFF.py:1139: in _check_merge_error
    with self.assertRaisesRegex(
E   AssertionError: "cube.attributes values differ for keys: 'a'" does not match "failed to merge into a single cube.
E     cube.attributes values differ for keys: "
=================================================================================== warnings summary ===================================================================================
lib/iris/tests/test_merge_NEWSTUFF.py::TestCubeMerge__split_attributes__error_messages::test_keys_differ__multiple
  <frozen importlib._bootstrap>:241: RuntimeWarning: numpy.ndarray size changed, may indicate binary incompatibility. Expected 16 from C header, got 96 from PyObject

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================================================== short test summary info ================================================================================
FAILED lib/iris/tests/test_merge_NEWSTUFF.py::TestCubeMerge__split_attributes__error_messages::test_splitattrs_keys_local_global_mismatch - AssertionError: "cube.attributes keys differ: 'a', 'b'" does not match "failed to merge into a single cube.
FAILED lib/iris/tests/test_merge_NEWSTUFF.py::TestCubeMerge__split_attributes__error_messages::test_splitattrs_keys_local_match_masks_global_mismatch - AssertionError: "cube.attributes values differ for keys: 'a'" does not match "failed to merge into a single cube.
======================================================================== 2 failed, 4 passed, 1 warning in 2.18s ========================================================================

NOTE: it was also noted in the above that sometimes the list of differing keys appears in a different order, due to use of set logic.
I have also now added sorted to fix that in a couple of places


Consult Iris pull request check list

@pp-mo
Copy link
Member Author

pp-mo commented Nov 17, 2023

@trexfeathers for your consideration ?
Enjoy !!

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

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

Comparison is base (0b54d58) 89.25% compared to head (58d48db) 89.46%.
Report is 143 commits behind head on FEATURE_split_attrs.

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    #5590      +/-   ##
=======================================================
+ Coverage                89.25%   89.46%   +0.21%     
=======================================================
  Files                       88       90       +2     
  Lines                    22197    22611     +414     
  Branches                  4858     5441     +583     
=======================================================
+ Hits                     19811    20228     +417     
+ Misses                    1641     1638       -3     
  Partials                   745      745              

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

@trexfeathers trexfeathers merged commit 40f82b5 into SciTools:FEATURE_split_attrs Nov 17, 2023
16 of 17 checks passed
ESadek-MO pushed a commit that referenced this pull request Nov 21, 2023
* 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 <[email protected]>

* Update lib/iris/cube.py

Co-authored-by: Martin Yeo <[email protected]>

* Update lib/iris/cube.py

Co-authored-by: Martin Yeo <[email protected]>

* Update lib/iris/cube.py

Co-authored-by: Martin Yeo <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* Update lib/iris/tests/unit/util/test_equalise_attributes.py

Co-authored-by: Martin Yeo <[email protected]>

* Simplify and clarify equalise_attributes code.

---------

Co-authored-by: Martin Yeo <[email protected]>

* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants