Array slicing bug fix #81
Conversation
|
Thanks for finding the bugs and making a start on this. Sorry to be a pain, but could you split this PR into smaller ones? As per the PR description, it's trying to do 3 things at once:
If you make a separate PR for each of those, we can have more focussed discussions on each one. It will make the reviewing process much easier. |
Adnan-Ali-Ahmad
changed the title
| @@ -47,7 +47,10 @@ def __getitem__(self, slice_): | |||
| unit=self._unit, | |||
| parent=self._parent, | |||
| name=self._name) | |||
| if isinstance(slice_, int) and self.ndim > 1: | |||
| if self.shape[0] == 1: | |||
There was a problem hiding this comment.
I am not so sure about this change...
When you slice with
data['amr']['position'][0], you get the single vector '' Min: 106.650 Max: 106.650 [au] (1, 3), which is what we wanted.
However, then if you slice that again, i.e. data['amr']['position'][0][0], one might expect to get the first component of the vector, but instead you get the 3 components as a normal array '' Min: 61.574 Max: 61.574 [au] (3,).
(you need to do data['amr']['position'][0][0][0] to get the first component...)
I agree that the previous behaviour, where data['amr']['position'][0][0] gives you the same as data['amr']['position'][0] is not much better. Maybe instead we should just prevent slicing single vectors further altogether, forcing the user to use the .x, .y, .z properties?
I guess no one prevents you from doing data['amr']['position'][0, 0] if you really want to get to a single number?
There was a problem hiding this comment.
Well if we want to be consistent with how numpy arrays work, it should return an array of shape (3,). So I'm not really sure about forcing the user to use the .x, .y and .z attributes.
There was a problem hiding this comment.
I am wondering if we should just revert the change and let it just behave like numpy?
And just let users use the data['amr']['position'][10:11] syntax to preserve the dimension?
There was a problem hiding this comment.
It's still nice to be able to retrieve a (1,3) shaped vector, so maybe we can do a revert on the __getitem__ operator and create a new method called get_vector(ind) that returns an array with shape (1,3) ?
There was a problem hiding this comment.
Since the previous change I made is still unreleased, I will revert that and just keep the [ind:ind+1] syntax for preserving the dimension.
We could always add a get_vector(ind) at a later stage, if more people request it.
Co-authored-by: Neil Vaytet <[email protected]>
|
See #90 |
There was a bug in #78 where slicing vectors with integers didn't work if
slice_was annp.integer(eg.np.argmax(data["hydro"]["density"]).valuesreturns anp.int64). Another bug was that if you sliced the array withdata["amr"]["position"][0]it returns an array with shape(1, 3)(as expected), however,data["amr"]["position"][0][0]returns(1, 3)again, when it should return(3, )(as before the PR, which allowed users to access the individual components of the vector). I implemented a quick shape and np.integer check.