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

Allow comparison between Cell and numpy array or list of len = 1. #4083

Merged
merged 5 commits into from Jul 23, 2021

Conversation

gcaria
Copy link
Contributor

@gcaria gcaria commented Apr 7, 2021

Fixes #2914.

A simple fix to allow numpy arrays (or lists) with only one element to be compared to a Cell.

@gcaria gcaria changed the title Allow comparison with numpy array or list of len = 1. Allow comparison between Cell and numpy array or list of len = 1. Apr 26, 2021
@bjlittle bjlittle added the Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton label May 19, 2021
@bjlittle bjlittle added this to In progress in Peloton via automation May 19, 2021
@bjlittle bjlittle moved this from In Progress to Backlog in Peloton May 19, 2021
@rcomer rcomer self-assigned this Jun 4, 2021
Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gcaria thanks for this and sorry nobody looked at it sooner. Your solution looks great for length 1 lists and arrays, but I'm afraid it doesn't quite address the case from #2914, as that involved a zero dimensioned array:

b = np.array(1.5)
len(b)

gives

TypeError: len() of unsized object

An alternative solution for numpy arrays might be to check other.size and use other.item() to get the value out. Or you might come up with a better idea - I don't want to be prescriptive!

It would also be good to add some simple tests to Test___common_cmp__ as a very basic check that you get the answer you expect for cases you have addressed (e.g. the example in #2914 should produce False).

Finally, it looks like the tests didn't actually run when you opened this PR. There has been a lot of effort since then to fix our testing infrastructure. So hopefully if you rebase it will get everything working sensibly again.

Again, thanks for the contribution!

@rcomer rcomer moved this from Backlog to In Progress in Peloton Jun 4, 2021
@gcaria gcaria force-pushed the cell_comparison_numpy_array branch from d60e042 to 27bf094 Compare July 16, 2021 13:17
Copy link
Contributor Author

@gcaria gcaria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback @rcomer!

I have edited the code to take care of arrays such as np.array(0), although it is arguably a bit of a misuse to not feed a sequence to np.array.

More than happy to add tests too, just wanted to check first if the casting to float sounds like a good idea (since the operations are <, >, <=, >= it seems fine to me).

@rcomer
Copy link
Member

rcomer commented Jul 16, 2021

Thanks for the update @gcaria!

it is arguably a bit of a misuse to not feed a sequence to np.array

I don't know how the original issue came up and unfortunately the developers involved have both since moved on. However I've found a couple of ways the 0-dimensional array could be created:

cube  = iris.cube.Cube(1)
print(repr(cube.data))

array = np.squeeze([[[4]]])
print(repr(array))

gives

array(1)
array(4)

so possibly there was a use-case where the 0-dimensional array came out of some other calculation.

Using float seems OK to me too given the operators.

@gcaria
Copy link
Contributor Author

gcaria commented Jul 18, 2021

Thanks for the clarification @rcomer, I was mostly surprised to see np.array can be used in that way, but this is surely not an issue with iris.

I have added a minimal set of lines for the testing.

@rcomer
Copy link
Member

rcomer commented Jul 19, 2021

Thanks @gcaria, these tests look good to me. Could you add similar tests for the size 1 array/list case you originally added? Just to make sure the various paths through the if loop are covered. Then this just needs an entry under "bugs fixed" in the whatsnew and I think we're done.

Really appreciate your work on this - it's great to get some of these old issues tidied up!

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great; thanks @gcaria!

@rcomer rcomer merged commit 149164e into SciTools:main Jul 23, 2021
Peloton automation moved this from In Progress to Done Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Peloton 🚴‍♂️ Target a breakaway issue to be caught and closed by the peloton Type: Bug
Projects
Development

Successfully merging this pull request may close these issues.

Cell comparison fails with 0-d numpy arrays
3 participants