-
-
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 11 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 |
---|---|---|
|
@@ -2,17 +2,17 @@ | |
''' | ||
Tests for NDCube | ||
''' | ||
from collections import namedtuple | ||
from collections import OrderedDict | ||
import datetime | ||
|
||
import pytest | ||
import sunpy.map | ||
import numpy as np | ||
import astropy.units as u | ||
|
||
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 +910,21 @@ 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, 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(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. As part of this test, could you also check the type of the output, and also that the meta is 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 was thinking that this test (taken from 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. Do you have an idea why Where |
||
inp_cube, inp_axis, inp_slice = test_input | ||
exp_dimensions, exp_type_seq, exp_meta_seq, exp_type_cube, exp_meta_cube = expected | ||
output = inp_cube.explode_along_axis(inp_axis) | ||
assert tuple(output.dimensions) == tuple(exp_dimensions) | ||
assert any(output[inp_slice].dimensions == \ | ||
u.Quantity((exp_dimensions[1], exp_dimensions[2]), unit='pix')) | ||
assert isinstance(output, exp_type_seq) | ||
assert isinstance(output[inp_slice], exp_type_cube) | ||
assert isinstance(output.meta, exp_meta_seq) | ||
assert isinstance(output[inp_slice].meta, exp_meta_cube) |
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 !