iris icon indicating copy to clipboard operation
iris copied to clipboard

Fix attribute array comparison

Open ukmo-ccbunney opened this issue 1 year ago • 2 comments

🚀 Pull Request

Description

Fixes: #6027

Logic for comparing arrays in attributes was relying on old numpy behaviour where an array compared to another any object would return a scalar True or False. Current numpy behaviour is to do a element-wise comparison which throws an error if the arrays are of different size.

Fix is to use the numpy.array_equal.


Consult Iris pull request check list

ukmo-ccbunney avatar Oct 18 '24 09:10 ukmo-ccbunney

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.83%. Comparing base (8c213f0) to head (50b624d). Report is 69 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6181      +/-   ##
==========================================
- Coverage   89.83%   89.83%   -0.01%     
==========================================
  Files          88       88              
  Lines       23211    23208       -3     
  Branches     4316     4316              
==========================================
- Hits        20852    20849       -3     
  Misses       1627     1627              
  Partials      732      732              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 18 '24 10:10 codecov[bot]

Note, this change fixes what I believe is a bug here: https://github.com/SciTools/iris/blob/d3071ff90f9e347ad4f1e1b9b097a74f28beb21d/lib/iris/_merge.py#L388-L391 Line 391 should be checking for any differences in the array elements, not all.

It also cleans up a now unnecessary try...except block that I believe was only needed for older versions of numpy. https://github.com/SciTools/iris/blob/d3071ff90f9e347ad4f1e1b9b097a74f28beb21d/lib/iris/common/mixin.py#L108-L111

I have extended the existing unit test to check for correct handling inequality. However, I question the validity of this test https://github.com/SciTools/iris/blob/d3071ff90f9e347ad4f1e1b9b097a74f28beb21d/lib/iris/tests/unit/common/mixin/test_LimitedAttributeDict.py#L45-L49 which now fails as it is testing a scalar value against a single element array. With the old logic, this would work, but it is flawed as np.all( np.array([0]) == 0 ) and np.all( np.zeros(100) == 0 ) would both return True, even though the latter is not what is intended.

I have commented out this test at the moment and would appreciate the opinion of @pp-mo.

ukmo-ccbunney avatar Oct 18 '24 10:10 ukmo-ccbunney