-
Notifications
You must be signed in to change notification settings - Fork 283
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
Fix attribute array comparison #6181
base: main
Are you sure you want to change the base?
Fix attribute array comparison #6181
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6181 +/- ##
==========================================
- Coverage 89.82% 89.81% -0.01%
==========================================
Files 88 88
Lines 23185 23182 -3
Branches 4314 4314
==========================================
- Hits 20825 20822 -3
Misses 1628 1628
Partials 732 732 ☔ View full report in Codecov by Sentry. |
Note, this change fixes what I believe is a bug here: Lines 388 to 391 in d3071ff
Line 391 should be checking for any differences in the array elements, not all. It also cleans up a now unnecessary Lines 108 to 111 in d3071ff
I have extended the existing unit test to check for correct handling inequality. iris/lib/iris/tests/unit/common/mixin/test_LimitedAttributeDict.py Lines 45 to 49 in d3071ff
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. |
…bunney/iris into bugfix/cube_attr_compare * 'bugfix/cube_attr_compare' of https://github.com/ukmo-ccbunney/iris: [pre-commit.ci] auto fixes from pre-commit.com hooks
# ChrisB: I am not sure the follow test is really valid. It is comparing | ||
# a one-element numpy array with a scalar. Is that what we want? | ||
|
||
# values = dict(one=np.arange(1), two=np.arange(1), three=np.arange(1)) | ||
# left = LimitedAttributeDict(dict(one=0, two=0, three=0)) | ||
# right = LimitedAttributeDict(**values) | ||
# self.assertEqual(left, right) | ||
# self.assertEqual(left, values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this usage is intentional.
We do want x to be the same as [x], because that's how netcdf attributes work.
So 1 = [1], but (obviously) 1 != [1, 0].
I think at least one way to handle this would be to wrap both sides with np.array(x, ndmin=1).
Interestingly, that leaves open the question of whether attributes can be multidimensional, or indeed non-array objects. Iris does allow that, but they can't exist in files: We use this. for example, in allowing attributes to be STASH objects -- but these are specifically intercepted + converted (both ways) on netcdf read and write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment where I think this changes too much from as-is behaviour.
Otherwise all good , I think
🚀 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
orFalse
. 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