First step to simplify NDCubeSequence slicing implementation #251

hayesla · 2020-04-02T20:08:42Z

First pass at #248

To Do

Add changelog file.
Fix any failing test(s).

pep8speaks · 2020-04-02T20:08:45Z

Hello @hayesla! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-13 03:58:08 UTC

DanRyanIrish · 2020-04-09T03:22:27Z

Hi @hayesla. I think this PR should also change the type of self.data to any array. I think the most efficient way to do this would be to change this line to self.data = np.asanyarray(data_list).

I don't think should cause any tests to fail, but if it does I think they should be simple enough fixes.

Also change the docstring accordingly, i.e. this line to

data: 1D `numpy.ndarray`
    Array of cubes

DanRyanIrish · 2020-04-09T03:30:59Z

It also looks like there is only one test that's failing: ndcube/tests/test_sequence_plotting.py:482

This PR also needs a trivial-type changelog, i.e. 251.trivial.rst in the changelog directory. See here for more instructions.

DanRyanIrish · 2020-04-09T13:30:22Z

@hayesla, could you move the code in __getitem__ to replace the code in utils.sequence.slice_sequence and then simply have __getitem__ return the output of utils.sequence.slice_sequence. This implementation is needed in sunraster.

hayesla · 2020-04-09T13:41:36Z

so do you mean that getitem just returns the utils.sequence.slice_sequence - as this works for ints, slices and tuples?

DanRyanIrish · 2020-04-09T13:50:34Z

Yep, that's right, e.g.

def __getitem__(self, item):
    return utils.sequence.slice_sequence(self, item)

DanRyanIrish · 2020-04-09T14:01:45Z

Added a To Do checklist in the PR description to show what's left to do. As you can see there is not much to do. I am considering doing a bugfix release soon, perhaps at the start of next week? Since this is needed for sunraster we could include it. The _common_axis slicing bug can be fixed in a separate PR and released as part of 2.0.

How does this sound?

DanRyanIrish · 2020-04-10T16:07:46Z

This commit doesn't do things the way I meant. I think this is my fault for a confusing explanation. I suggest you wind back this commit and proceed without it. I'll remove the slice_sequence step in the above to do list too.

hayesla · 2020-04-10T19:55:19Z

@DanRyanIrish - I think this is more along the lines you were thinking?

It still don't fix the common_axis issue but as you mentioned maybe that could go in another PR?

DanRyanIrish

This looks good!

DanRyanIrish · 2020-04-10T20:17:44Z

ndcube/ndcube_sequence.py

@@ -84,14 +84,21 @@ def cube_like_world_axis_physical_types(self):
        return self.data[0].world_axis_physical_types

    def __getitem__(self, item):
+


Suggested change

DanRyanIrish · 2020-04-10T20:19:43Z

changelog/251.trivial.rst

@@ -0,0 +1 @@
+use the `utils.sequence.slice_sequence` for __getitem__


Suggested change

use the `utils.sequence.slice_sequence` for __getitem__

Simplify and speed up implementation of NDCubeSequence slicing.

DanRyanIrish · 2020-04-10T20:24:23Z

It still don't fix the common_axis issue but as you mentioned maybe that could go in another PR?

Yes, let's keep that for a separate PR.

DanRyanIrish · 2020-04-10T20:27:23Z

ndcube/ndcube_sequence.py

            result = copy.deepcopy(self)
-            result.data = self.data[item]
+            if isinstance(item, slice):
+                    result.data = self.data[item]


Suggested change

result.data = self.data[item]

result.data = self.data[item]

Over-indented line.

DanRyanIrish · 2020-04-10T20:28:35Z

Hmmm. I don't understand those test failures... Perhaps @nabobalis can interpret them for us?

DanRyanIrish · 2020-04-10T20:43:20Z

To get the tests to behave correctly, we have to pin pytest to <5.4. To do this change this line in setup.cfg to pytest < 5.4. Then commit and push.

Be sure to make this the only change in the commit. It makes it easy to cherry-pick later it I need it for the bugfix release. Thanks!!

hayesla · 2020-04-10T20:47:47Z

done!

DanRyanIrish · 2020-04-10T20:52:39Z

Great! Hopefully that'll make the test fails more understandable.

nabobalis · 2020-04-10T20:58:15Z

Seems to be:

===================================================================================== ERRORS ======================================================================================
_________________________________________________ ERROR collecting .tox/py38/Lib/site-packages/ndcube/tests/test_ndcollection.py __________________________________________________
..\..\.tox\py38\Lib\site-packages\ndcube\tests\test_ndcollection.py:77: in <module>
    NDCollection([("seq0", sequence02[:, 1, 1:3]), ("seq1", sequence20[:, 1, 1:3])],
..\..\.tox\py38\Lib\site-packages\ndcube\ndcube_sequence.py:97: in __getitem__
    elif isinstance(item[0], slice) and item[0].stop - item[0].start == 1:
E   TypeError: unsupported operand type(s) for -: 'NoneType' and 'NoneType'

The mac os build failed due to threading, so we will need to add posargs: -n=1 to the last line of the mac os block in the azure template.

hayesla · 2020-04-10T21:03:30Z

Seems to be:

===================================================================================== ERRORS ======================================================================================
_________________________________________________ ERROR collecting .tox/py38/Lib/site-packages/ndcube/tests/test_ndcollection.py __________________________________________________
..\..\.tox\py38\Lib\site-packages\ndcube\tests\test_ndcollection.py:77: in <module>
    NDCollection([("seq0", sequence02[:, 1, 1:3]), ("seq1", sequence20[:, 1, 1:3])],
..\..\.tox\py38\Lib\site-packages\ndcube\ndcube_sequence.py:97: in __getitem__
    elif isinstance(item[0], slice) and item[0].stop - item[0].start == 1:
E   TypeError: unsupported operand type(s) for -: 'NoneType' and 'NoneType'

The mac os build failed due to threading, so we will need to add posargs: -n=1 to the last line of the mac os block in the azure template.

I think this may be an actually bug in the code no?

nabobalis · 2020-04-10T21:06:10Z

I would say yes?

DanRyanIrish · 2020-04-10T21:08:52Z

ndcube/ndcube_sequence.py

+            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:


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.

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

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

Ah! You're dead right!

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:]]]

ndcube/ndcube_sequence.py

DanRyanIrish · 2020-04-11T19:58:54Z

So this code passes tests now with the exception of one doctest in ndcubesequence.rst. However, when I run the same code locally after pulling @hayesla's branch, it gives the right output. I can't understand why that is! @nabobalis have you ever seen that before?

Comparison of test output:
Azure case:

189 independently!) However, with `~ndcube.NDCubeSequence` this becomes as
190 simple as indexing a single array::
191 
192   >>> regions_of_interest_in_sequence = my_sequence[1:3, 0, 1:3, 1:4]
193   >>> regions_of_interest_in_sequence.dimensions
Expected:
    (<Quantity 2. pix>, <Quantity 2. pix>, <Quantity 3. pix>)
Got:
    (<Quantity 3. pix>, <Quantity 3. pix>, <Quantity 4. pix>, <Quantity 5. pix>)

/home/vsts/work/1/s/docs/ndcubesequence.rst:193: DocTestFailure

Output when executing code locally:

>>> regions_of_interest_in_sequence = my_sequence[1:3, 0, 1:3, 1:4]
>>> regions_of_interest_in_sequence.dimensions
(<Quantity 2. pix>, <Quantity 2. pix>, <Quantity 3. pix>)  # This is the same as what the doctest expects.

nabobalis · 2020-04-11T20:46:11Z

I am not sure. There has to be some difference but what I can't say for now.

DanRyanIrish

Just realised why this was failing. The case where item[0] is a slice but stop - start != 1 is not handled!

ndcube/ndcube_sequence.py

The case where the slice item was a tuple, item[0] is a slice of length > 1 so simply dropped. This fixes that oversight.

ndcube/ndcube_sequence.py

DanRyanIrish · 2020-04-13T04:11:40Z

Hi @hayesla. I got this PR working. Hooray!!!

Before merging, could I ask you to squash all the commits? If you aren't sure how to do that we can walk through it together. Making this a 1 commit PR will make cherry-picking it for the release of 1.3.1 much easier.

DanRyanIrish · 2020-04-13T14:33:30Z

Thanks for this successful PR @hayesla!!

hayesla · 2020-04-13T14:56:11Z

whoop whoop first NDCube PR 👯

First step to simplify NDCubeSequence slicing implementation

first step to simplify NDCubeSequence slicing implementation

1504983

DanRyanIrish added this to the 2.0 milestone Apr 3, 2020

debug

bf9b87a

DanRyanIrish mentioned this pull request Apr 9, 2020

Iterating over NDCubeSequence very slow with many "large" NDCubes #190

Closed

DanRyanIrish modified the milestones: 2.0, 1.3.1 Apr 9, 2020

adding changelog

7509ca9

hayesla added 2 commits April 10, 2020 15:42

adding slicing capabilitiy to __getitem__

5ceb483

Merge branch 'master' into ndcube_sequence_slicing

231c641

hayesla force-pushed the ndcube_sequence_slicing branch from 18f88fb to 231c641 Compare April 10, 2020 19:45

DanRyanIrish reviewed Apr 10, 2020

View reviewed changes

DanRyanIrish modified the milestones: 1.3.1, 2.0 Apr 10, 2020

DanRyanIrish reviewed Apr 10, 2020

View reviewed changes

hayesla added 2 commits April 10, 2020 16:45

review comments update

612a3cd

pin pytest to <5.4

ebff0bf

DanRyanIrish reviewed Apr 10, 2020

View reviewed changes

DanRyanIrish modified the milestones: 2.0, 1.3.1 Apr 11, 2020

DanRyanIrish mentioned this pull request Apr 11, 2020

Feature list for v1.3.1 #263

Closed

hayesla added 2 commits April 11, 2020 14:56

Merge branch 'master' into ndcube_sequence_slicing

74f5f12

adding updates from review

e2e6d33

DanRyanIrish reviewed Apr 11, 2020

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Typo bugfix

c11a0a1

DanRyanIrish requested changes Apr 13, 2020

View reviewed changes

Add missing case in NDCubeSequence handling.

aaae312

The case where the slice item was a tuple, item[0] is a slice of length > 1 so simply dropped. This fixes that oversight.

DanRyanIrish requested changes Apr 13, 2020

View reviewed changes

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

ndcube/ndcube_sequence.py Outdated Show resolved Hide resolved

Fix indentation

6d0c9a0

DanRyanIrish merged commit b5db986 into sunpy:master Apr 13, 2020

DanRyanIrish added a commit to DanRyanIrish/ndcube that referenced this pull request Apr 13, 2020

Merge pull request sunpy#251 from hayesla/ndcube_sequence_slicing

c24fe2e

First step to simplify NDCubeSequence slicing implementation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First step to simplify NDCubeSequence slicing implementation #251

First step to simplify NDCubeSequence slicing implementation #251

hayesla commented Apr 2, 2020 •

edited by DanRyanIrish

Loading

pep8speaks commented Apr 2, 2020 •

edited

Loading

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020 •

edited

Loading

hayesla commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 10, 2020

hayesla commented Apr 10, 2020

DanRyanIrish left a comment

DanRyanIrish Apr 10, 2020

DanRyanIrish Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

DanRyanIrish Apr 10, 2020

DanRyanIrish Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

hayesla commented Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

nabobalis commented Apr 10, 2020 •

edited

Loading

hayesla commented Apr 10, 2020

nabobalis commented Apr 10, 2020

DanRyanIrish Apr 10, 2020

hayesla Apr 10, 2020

hayesla Apr 10, 2020

DanRyanIrish Apr 11, 2020

DanRyanIrish Apr 11, 2020

DanRyanIrish commented Apr 11, 2020

nabobalis commented Apr 11, 2020

DanRyanIrish left a comment

DanRyanIrish commented Apr 13, 2020

DanRyanIrish commented Apr 13, 2020

hayesla commented Apr 13, 2020

		@@ -84,14 +84,21 @@ def cube_like_world_axis_physical_types(self):
		return self.data[0].world_axis_physical_types

		def __getitem__(self, item):

		@@ -0,0 +1 @@
		use the `utils.sequence.slice_sequence` for __getitem__

	use the `utils.sequence.slice_sequence` for __getitem__
	Simplify and speed up implementation of NDCubeSequence slicing.

-                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:]]]

First step to simplify NDCubeSequence slicing implementation #251

First step to simplify NDCubeSequence slicing implementation #251

Conversation

hayesla commented Apr 2, 2020 • edited by DanRyanIrish Loading

pep8speaks commented Apr 2, 2020 • edited Loading

Comment last updated at 2020-04-13 03:58:08 UTC

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020 • edited Loading

hayesla commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 9, 2020

DanRyanIrish commented Apr 10, 2020

hayesla commented Apr 10, 2020

DanRyanIrish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

hayesla commented Apr 10, 2020

DanRyanIrish commented Apr 10, 2020

nabobalis commented Apr 10, 2020 • edited Loading

hayesla commented Apr 10, 2020

nabobalis commented Apr 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 11, 2020

nabobalis commented Apr 11, 2020

DanRyanIrish left a comment

Choose a reason for hiding this comment

DanRyanIrish commented Apr 13, 2020

DanRyanIrish commented Apr 13, 2020

hayesla commented Apr 13, 2020

hayesla commented Apr 2, 2020 •

edited by DanRyanIrish

Loading

pep8speaks commented Apr 2, 2020 •

edited

Loading

DanRyanIrish commented Apr 9, 2020 •

edited

Loading

nabobalis commented Apr 10, 2020 •

edited

Loading