Adding a new explode_along_axis function inside NDCube #118

BaptistePellorceAstro · 2018-05-04T10:08:19Z

Here the first version of explode_along_axis inside NDCube.

pep8speaks · 2018-05-04T10:08:22Z

Hello @BaptistePellorceAstro! Thanks for updating the PR.

In the file ndcube/tests/test_ndcube.py, following are the PEP8 issues :

Line 926:9: E128 continuation line under-indented for visual indent
Line 925:48: E502 the backslash is redundant between brackets

Comment last updated on May 18, 2018 at 15:08 Hours UTC

DanRyanIrish · 2018-05-04T15:39:43Z

ndcube/ndcube.py

+    def explode_along_axis(self, axis):
+        """
+        Separates slices of NDCubes in sequence along a given cube axis into a NDCubeSequence
+        of (N-1)DCubes.


Docstring should reflect that this is an NDCube method, not an NDCubeSequence. So replace with something like:
Separates slices of the NDCube along a given cube axis into a NDCubeSequence of (N-1)DCubes

Oh yes, of course !

DanRyanIrish · 2018-05-04T16:05:35Z

ndcube/ndcube.py

+                meta=self.meta, mask=self[i].mask, missing_axis=[False, False, True],
+                extra_coords=extra_coords_formated)
+            result_cubes.append(cube)
+        return NDCubeSequence(result_cubes, meta=self.meta)


I think this method could be rewritten with the following:

result_cubes = [] cube_slices = [slice(None, None, None)] * len(self.data.ndim) for i in range(self.data.shape[axis]): cube_slices[axis] = i result_cubes.append(self[cube_slices]) return NDCubeSequence(result_cubes, common_axis=axis)

You make me understand these lines just now ! Of course, this is the best way.

DanRyanIrish · 2018-05-04T16:07:19Z

Thanks so much for this PR @BaptistePellorceAstro. I've left a couple comments. As well as those, this method should be in NDCubeBase, rather than NDCube. It will then get inherited by NDCube.

DanRyanIrish · 2018-05-07T12:47:29Z

ndcube/ndcube.py

@@ -542,34 +539,24 @@ def explode_along_axis(self, axis):
        result : `ndcube_sequence.NDCubeSequence`

        """
-        # If axis is -ve then calculate the axis from the length of the dimensions of one cube
+        # is axis is -ve then calculate the axis from the length of the dimensions of one cube


Typo: Should be If.

I modified it but I copy the previous line ... a little mistake.

DanRyanIrish · 2018-05-07T13:08:58Z

ndcube/ndcube.py

+            # Setting the slice value to the index so that the slices are done correctly.
+            cube_slices[axis] = i
+            # Appending the sliced cubes in the result_cube list
+            result_cubes.append(self[cube_slices])


The way this is arranged now, the metadata of the Cube will be copied for each slice. Really, the metadata still applies to all the data and isn't specific to the slices. So I think the metadata should be moved from the cube level, to the sequence level. This could be done by doing something like:

sliced_cube = self[cube_slices] sliced_cube.meta = None result_cubes.append(self[cube_slices]) return NDCubeSequence(result_cubes, common_axis=axis, meta=self.meta)

DanRyanIrish · 2018-05-07T13:10:50Z

Hi @BaptistePellorceAstro. This is looking very good now. Now, as well as the tiny changes I just requested, this PR needs tests! :)

DanRyanIrish · 2018-05-08T12:31:41Z

ndcube/ndcube.py

            sliced_cube.meta = None
            # Appending the sliced cubes in the result_cube list
-            result_cubes.append(self[cube_slices])
+            result_cubes.append(self[item])


self[item] here should be sliced_cube.

Of course, I forget to modify this one when I created item to solve a bug.

DanRyanIrish · 2018-05-08T12:33:11Z

ndcube/tests/test_ndcube.py

+    (cubem.explode_along_axis(1), (3*u.pix, 2*u.pix, 4*u.pix)),
+    (cubem.explode_along_axis(-2), (3*u.pix, 2*u.pix, 4*u.pix))
+])
+def test_explode_along_axis(test_input, expected):


As part of this test, could you also check the type of the output, and also that the meta is None for the sub-cubes and the the sequence meta is the same as the original cube meta?

Yes ! I was thinking that this test (taken from ndcube_sequence) is not relevant at all.

Do you have an idea why NDCube.explode_along_axis(axis=0)[0].meta is an OrderedDict() ?
It should not be None ?

Where NDCube.explode_along_axis(axis=0) is a NDCubeSequence and NDCube.explode_along_axis(axis=0)[0] the first NDCube of the previous NDCubeSequence.

BaptistePellorceAstro · 2018-05-08T14:19:21Z

ndcube/tests/test_ndcube.py

+    (cubem.explode_along_axis(0).dimensions, (2*u.pix, 3*u.pix, 4*u.pix)),
+    (cubem.explode_along_axis(1).dimensions, (3*u.pix, 2*u.pix, 4*u.pix)),
+    (cubem.explode_along_axis(-2).dimensions, (3*u.pix, 2*u.pix, 4*u.pix)),
+    (cubem.explode_along_axis(0)[0].meta, OrderedDict()),


Is really an OrderedDict() ???

No, it should be an NDCubeSequence.

No I am speaking about the NDCube.meta

Ah! Well I think recently in Python, dictionaries now default to OrderedDict. So I think that's fine.

DanRyanIrish · 2018-05-09T12:37:21Z

ndcube/tests/test_ndcube.py

@@ -910,3 +912,15 @@ def test_axis_world_coords_without_input(test_input, expected):
    for i in range(len(all_coords)):
        np.testing.assert_allclose(all_coords[i].value, expected[i].value)
        assert all_coords[i].unit == expected[i].unit
+
+
+@pytest.mark.parametrize("test_input,expected", [


You can put in more than one expected output by making expected a tuple and then unpacking that tuple in the test. The test should be testing the same thing(s) in the same way. You can test multiple things in one test.

Yes I know. Do you want I regroup types test ?

I think it's fine to have something like:

def test_(): expected_dimensions, expected_type, ... = *expected assert expected_dimensions == test_input.dimensions assert isinstance(test_input, expected_type) ...

Ok, I go change that.

I must add and other test for the metadata.

I clearly understand why you want to regroup tests but I didn't find a better than seperate NDCubeSequence and sub-NDCube. Like that, we can have a better understanding about which test is about which types. But I think that will be interesting to modify the quantity of list for the sub-NDCube.dimensions to a tuple of quantity as for NDCubeSequence.

Just to clarify, you're saying NDCube.dimensions should give (<Quantity x pix>, <Quantity x pix>, <Quantity x pix>) instead of what it current gives which is <Quantity [x, x, x] pix>?

Yes, I think this should be better, as we try to have the same functions for each class.

But maybe be the list is used in some other functions ...

I would be open to considering that. Having a Quantity of multiple numbers is obviously handier. But you can't do that for NDCubeSequence as if there's a common axis, the length of the cubes along that axis many not be the same length. On the other hand, you could turn a tuple of single value quantities into a single quantity of multiple values by doing u.Quantity((0*u.pix, 1*u.pix), unit=u.pix). Although it's probably not very fast.

Like I said, I'm open to it. I'll think about it a little more. :)

@Cadair could we have your thoughts on this proposed change to the NDCube.dimensions API? The proposed change is that the output will become a tuple of length-one Quantities, rather than a Quantity of length>1. You can see @BaptistePellorceAstro's original arguments above. I am open to it for a couple of reasons:

It is more consistent with the output of ndarray.shape and other such attributes;

It makes NDCube.dimensions consistent with NDCubeSequence.dimensions. The latter is a tuple of Quantities so if the sub-cubes' along the common axis are of differing lengths, that can be expressed by the common axis Quantity in the tuple having length > 1 which gives the length of each cube.

It can be easily converted to a single Quantity by doing >>> Quantity(my_cube.dimensions, unit="pix")

This may however, have repercussion for APE 14? If so, perhaps there are arguments to escalate this change there too?

BaptistePellorceAstro · 2018-05-14T12:28:58Z

I succeed to do only one test for explode_along_axis.

DanRyanIrish · 2018-05-14T14:28:14Z

ndcube/tests/test_ndcube.py

-def test_explode_along_axis_meta(test_input, expected):
-    expected_type, expected_meta = expected
+def test_explode_along_axis_v2(test_input, expected):
+    expected_dimensions, expected_type, expected_meta = expected


The original NDCube and the axis along which is should be exploded should be in the parameterisation and the the method on them, explode_along_axis should be called in the test:

output = test_input[0].explode_along_axis(test_input[1])

DanRyanIrish · 2018-05-14T14:29:04Z

ndcube/tests/test_ndcube.py

-])
-def test_explode_along_axis_meta(test_input, expected):
-    expected_type, expected_meta = expected
+def test_explode_along_axis_v2(test_input, expected):


No need for a _v2 in the test name here. That's what version control is for :)

That was an old test in my local repo =)

DanRyanIrish · 2018-05-15T13:32:30Z

ndcube/tests/test_ndcube.py

-    assert isinstance(test_input, expected_type)
-    assert isinstance(test_input.meta, expected_meta)
+def test_explode_along_axis(test_input, expected):
+    output = test_input[0].explode_along_axis(test_input[1])


An optional change: Since you now have 3 inputs, you could do the same thing as you did with the expected outputs and unpack them into sensible variables like:

input_cube, input_axis, test_slice = *test_input

But this a matter of your personal preference in coding style and so optional.

This is clearer as you said so I modified.

DanRyanIrish · 2018-05-15T16:11:05Z

Hi @BaptistePellorceAstro. I'm happy with these changes. The PR looks great. Let me know if you want to make any further optional additions, like the one I mentioned in the most recent comment. If not, I'll merge the PR :)

BaptistePellorceAstro · 2018-05-16T09:28:20Z

If you are OK with the new version, I am ready to merge.

DanRyanIrish · 2018-05-16T11:52:48Z

I think this PR is almost ready to go. It just has to be altered so the tests pass.

https://travis-ci.org/sunpy/ndcube/jobs/379626836

BaptistePellorceAstro · 2018-05-16T12:03:35Z

I don't understand what is this problem. In my local repo, the test succeeded.

DanRyanIrish · 2018-05-17T03:07:52Z

Maybe time to call on @Cadair, the guru of understanding Travis errors. Any interpretation of what these errors actually mean, @Cadair?

BaptistePellorceAstro · 2018-05-18T11:58:19Z

I succeeded to solve Travis error, I am ready to merge.

DanRyanIrish · 2018-05-18T14:54:47Z

@BaptistePellorceAstro, I just merged a bugfix PR from @Cadair . Could you pull the latest version of master in your explode_along_axis branch and push it to this PR. Once that's done and there are no test failures, I'll merge.

BaptistePellorceAstro · 2018-05-18T15:00:42Z

I am not at the office, can I do it from the website ?

Update of explode_along_axis before a PR

BaptistePellorceAstro · 2018-05-18T15:11:12Z

I tried something, it is what you want ?

DanRyanIrish · 2018-05-18T15:32:39Z

Yep! That looks good! 👍

DanRyanIrish · 2018-05-18T15:32:55Z

Another PR merged!!!

First version of explode along axis

dc72409

Removing blank line

2b4b1a1

DanRyanIrish reviewed May 4, 2018

View reviewed changes

Some code modifications

4f98fb3

DanRyanIrish requested changes May 7, 2018

View reviewed changes

BaptistePellorceAstro added 2 commits May 7, 2018 15:20

Removing metadata from sliced cubes

b12fa3b

Test for explode_along_axis

ad658d3

DanRyanIrish requested changes May 8, 2018

View reviewed changes

Improving tests

a588c34

BaptistePellorceAstro commented May 8, 2018

View reviewed changes

DanRyanIrish reviewed May 9, 2018

View reviewed changes

BaptistePellorceAstro added 2 commits May 9, 2018 16:17

Improving tests

4a68b34

Improving tests v2

e865984

DanRyanIrish reviewed May 14, 2018

View reviewed changes

Improving test v3

b9c09d9

DanRyanIrish reviewed May 15, 2018

View reviewed changes

DanRyanIrish approved these changes May 15, 2018

View reviewed changes

Inputs extraction in test

37dd4ef

DanRyanIrish mentioned this pull request May 17, 2018

SJICube Refactor sunpy/sunraster#66

Closed

Trying solve Travis error

dcf1540

DanRyanIrish approved these changes May 18, 2018

View reviewed changes

Merge pull request #1 from sunpy/master

1d43c63

Update of explode_along_axis before a PR

DanRyanIrish merged commit c72df57 into sunpy:master May 18, 2018

BaptistePellorceAstro deleted the explode_along_axis branch May 25, 2018 12:12

Adding a new explode_along_axis function inside NDCube #118

Adding a new explode_along_axis function inside NDCube #118

Conversation

BaptistePellorceAstro commented May 4, 2018

pep8speaks commented May 4, 2018 • edited Loading

Comment last updated on May 18, 2018 at 15:08 Hours UTC

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 May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanRyanIrish May 7, 2018 • edited Loading

Choose a reason for hiding this comment

DanRyanIrish commented May 7, 2018

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

BaptistePellorceAstro May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaptistePellorceAstro May 9, 2018 • edited Loading

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

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

BaptistePellorceAstro May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BaptistePellorceAstro commented May 14, 2018

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 May 15, 2018

BaptistePellorceAstro commented May 16, 2018

DanRyanIrish commented May 16, 2018

BaptistePellorceAstro commented May 16, 2018 • edited Loading

DanRyanIrish commented May 17, 2018

BaptistePellorceAstro commented May 18, 2018 • edited Loading

DanRyanIrish commented May 18, 2018

BaptistePellorceAstro commented May 18, 2018

BaptistePellorceAstro commented May 18, 2018

DanRyanIrish commented May 18, 2018

DanRyanIrish commented May 18, 2018

pep8speaks commented May 4, 2018 •

edited

Loading

DanRyanIrish commented May 4, 2018 •

edited

Loading

DanRyanIrish May 7, 2018 •

edited

Loading

BaptistePellorceAstro May 8, 2018 •

edited

Loading

BaptistePellorceAstro May 9, 2018 •

edited

Loading

BaptistePellorceAstro May 10, 2018 •

edited

Loading

BaptistePellorceAstro commented May 16, 2018 •

edited

Loading

BaptistePellorceAstro commented May 18, 2018 •

edited

Loading