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

Adding a new explode_along_axis function inside NDCube #118

Merged
merged 12 commits into from
May 18, 2018
Merged

Adding a new explode_along_axis function inside NDCube #118

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
dc72409
First version of explode along axis
BaptistePellorceAstro May 4, 2018
2b4b1a1
Removing blank line
BaptistePellorceAstro May 4, 2018
4f98fb3
Some code modifications
BaptistePellorceAstro May 7, 2018
b12fa3b
Removing metadata from sliced cubes
BaptistePellorceAstro May 7, 2018
ad658d3
Test for explode_along_axis
BaptistePellorceAstro May 8, 2018
a588c34
Improving tests
BaptistePellorceAstro May 8, 2018
4a68b34
Improving tests
BaptistePellorceAstro May 9, 2018
e865984
Improving tests v2
BaptistePellorceAstro May 14, 2018
b9c09d9
Improving test v3
BaptistePellorceAstro May 15, 2018
37dd4ef
Inputs extraction in test
BaptistePellorceAstro May 16, 2018
dcf1540
Trying solve Travis error
BaptistePellorceAstro May 18, 2018
1d43c63
Merge pull request #1 from sunpy/master
BaptistePellorceAstro May 18, 2018
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
21 changes: 7 additions & 14 deletions ndcube/tests/test_ndcube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,20 +915,13 @@ def test_axis_world_coords_without_input(test_input, expected):


@pytest.mark.parametrize("test_input,expected", [
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I go change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must add and other test for the metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@BaptistePellorceAstro BaptistePellorceAstro May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 ...

Copy link
Member

Choose a reason for hiding this comment

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

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. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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?

(cubem.explode_along_axis(0), ((2*u.pix, 3*u.pix, 4*u.pix), NDCubeSequence)),
(cubem.explode_along_axis(1), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence)),
(cubem.explode_along_axis(-2), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence)),
(cubem.explode_along_axis(0), ((2*u.pix, 3*u.pix, 4*u.pix), NDCubeSequence, dict)),
(cubem.explode_along_axis(1), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence, dict)),
(cubem.explode_along_axis(-2), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence, dict)),
(cubem.explode_along_axis(0)[0], ([3., 4.]*u.pix, NDCube, OrderedDict))
])
def test_explode_along_axis(test_input, expected):
expected_dimensions, expected_type = expected
assert test_input.dimensions == expected_dimensions
assert isinstance(test_input, expected_type)


@pytest.mark.parametrize("test_input,expected", [
(cubem.explode_along_axis(0)[0], (NDCube, OrderedDict))
])
def test_explode_along_axis_meta(test_input, expected):
expected_type, expected_meta = expected
def test_explode_along_axis_v2(test_input, expected):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was an old test in my local repo =)

expected_dimensions, expected_type, expected_meta = expected
Copy link
Member

Choose a reason for hiding this comment

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

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

assert tuple(test_input.dimensions) == tuple(expected_dimensions)
assert isinstance(test_input, expected_type)
assert isinstance(test_input.meta, expected_meta)