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.

Sign up for GitHub

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

Array slicing bug fix #81

Closed
wants to merge 8 commits into from
Closed

Array slicing bug fix #81

Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f26433e
added spatial.py & fixed slicing bug in array.py
Adnan-Ali-Ahmad Feb 17, 2022
8790f8e
forgot yapf formatting
Adnan-Ali-Ahmad Feb 17, 2022
6b2824e
flake8
Adnan-Ali-Ahmad Feb 17, 2022
7888bc0
last typo fixed
Adnan-Ali-Ahmad Feb 17, 2022
41155c7
removed redundant function and normalized guiding vector in rotation_…
Adnan-Ali-Ahmad Feb 17, 2022
3bdee6d
slicing up PR
Adnan-Ali-Ahmad Feb 18, 2022
90c6381
forgot to remove spatial import
Adnan-Ali-Ahmad Feb 18, 2022
2e938a1
Update src/osyris/core/array.py
Adnan-Ali-Ahmad Feb 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/osyris/core/array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

return out
if (isinstance(slice_, int)
or isinstance(slice_, np.integer)) and self.ndim > 1:
Adnan-Ali-Ahmad marked this conversation as resolved.
Show resolved Hide resolved
return out.reshape(1, len(out))
return out

Expand Down