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

First step to simplify NDCubeSequence slicing implementation #251

Merged
merged 12 commits into from
Apr 13, 2020
1 change: 1 addition & 0 deletions changelog/251.trivial.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Simplify and speed up implementation of NDCubeSequence slicing.
15 changes: 11 additions & 4 deletions ndcube/ndcube_sequence.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,21 @@ def cube_like_world_axis_physical_types(self):
return self.data[0].world_axis_physical_types

def __getitem__(self, item):

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

if isinstance(item, numbers.Integral):
return self.data[item]
elif isinstance(item, slice):
else:
result = copy.deepcopy(self)
result.data = self.data[item]
if isinstance(item, slice):
result.data = self.data[item]
else:
if isinstance(item[0], numbers.Integral):
result = result.data[item[0]][item[1:]]
elif isinstance(item[0], slice) and item[0].stop - item[0].start == 1:
Copy link
Member

Choose a reason for hiding this comment

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

Just looked at this again. I don't think this line and the one below are needed at all? The line after the else will do the same job in all remaining cases, I'm pretty sure? If so, that'll also get rid of the test failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah its needed, as if you just do the list comprehension on the NDCube rather on the list of NDCubes it then loops through the arrays in the NDCube if that makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

i.e. as in for this result.data = [cube[item[1:]] for cube in result.data[item[0]]] to work you need result.data[item[0]] to be a list of NDCubes rather than an NDCube. but if item[0] is a 0, or slice(0, 1) this doesn't work ... the way it is currently implemented also follows similar logic to the way the utils.sequence.slice_sequence kind of did it without the need of a SequenceItem

Copy link
Member

Choose a reason for hiding this comment

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

Ah! You're dead right!

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
elif isinstance(item[0], slice) and item[0].stop - item[0].start == 1:
elif isinstance(item[0], slice):
start = 0 if item[0].start is None else item[0].start
stop = len(self.data) if item[0].stop is None else item[0].stop
if stop - start == 1:
result.data = [result.data[item[0].start][item[1:]]]

result.data = [result.data[item[0].start][item[1:]]]
else:
DanRyanIrish marked this conversation as resolved.
Show resolved Hide resolved
result.data = [cube[item[1:]] for cube in result.data[item[0]]]
DanRyanIrish marked this conversation as resolved.
Show resolved Hide resolved
return result
else:
return utils.sequence.slice_sequence(self, item)

@property
def index_as_cube(self):
Expand Down
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ install_requires =

[options.extras_require]
test =
pytest
pytest < 5.4
pytest-astropy
docs =
sphinx
Expand Down