-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Adding a new explode_along_axis function inside NDCube #118
Changes from 8 commits
dc72409
2b4b1a1
4f98fb3
b12fa3b
ad658d3
a588c34
4a68b34
e865984
b9c09d9
37dd4ef
dcf1540
1d43c63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
Tests for NDCube | ||
''' | ||
from collections import namedtuple | ||
from collections import OrderedDict | ||
import datetime | ||
|
||
import pytest | ||
|
@@ -13,6 +14,7 @@ | |
from ndcube import NDCube, NDCubeOrdered | ||
from ndcube.utils.wcs import WCS, _wcs_slicer | ||
from ndcube.tests import helpers | ||
from ndcube.ndcube_sequence import NDCubeSequence | ||
|
||
# sample data for tests | ||
# TODO: use a fixture reading from a test file. file TBD. | ||
|
@@ -910,3 +912,16 @@ 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", [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can put in more than one expected output by making There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I know. Do you want I regroup types test ? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I go change that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I must add and other test for the metadata. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to clarify, you're saying There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Like I said, I'm open to it. I'll think about it a little more. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Cadair could we have your thoughts on this proposed change to the
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)) | ||
]) | ||
def test_explode_along_axis_v2(test_input, expected): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring should reflect that this is an
NDCube
method, not anNDCubeSequence
. So replace with something like:Separates slices of the NDCube along a given cube axis into a NDCubeSequence of (N-1)DCubes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, of course !