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
22 changes: 13 additions & 9 deletions ndcube/tests/test_ndcube.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,13 +915,17 @@ 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, 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))
((cubem, 0, 0), ((2*u.pix, 3*u.pix, 4*u.pix), NDCubeSequence, dict, NDCube, OrderedDict)),
((cubem, 1, 0), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence, dict, NDCube, OrderedDict)),
((cubem, -2, 0), ((3*u.pix, 2*u.pix, 4*u.pix), NDCubeSequence, dict, NDCube, OrderedDict))
])
def test_explode_along_axis_v2(test_input, expected):
expected_dimensions, expected_type, expected_meta = expected
assert tuple(test_input.dimensions) == tuple(expected_dimensions)
assert isinstance(test_input, expected_type)
assert isinstance(test_input.meta, expected_meta)
def test_explode_along_axis(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.

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?

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 was thinking that this test (taken from ndcube_sequence) is not relevant at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

output = test_input[0].explode_along_axis(test_input[1])
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is clearer as you said so I modified.

exp_dimensions, exp_type_seq, exp_meta_seq, exp_type_cube, exp_meta_cube = expected
assert tuple(output.dimensions) == tuple(exp_dimensions)
assert any(output[test_input[2]].dimensions == \
u.Quantity((exp_dimensions[1], exp_dimensions[2]), unit='pix'))
assert isinstance(output, exp_type_seq)
assert isinstance(output[test_input[2]], exp_type_cube)
assert isinstance(output.meta, exp_meta_seq)
assert isinstance(output[test_input[2]].meta, exp_meta_cube)