Allow length-1 inputs to NDCube.crop #863
Conversation
Allow length-1 inputs and convert them to scalars.
DanRyanIrish
requested review from
hayesla,
nabobalis and
wtbarnes
and removed request for
nabobalis
|
i do think this was an issue raised by the map refactor that i found, can you remind me again what the issue was (not clear from the linked issue) |
| def test_crop_length_1_input(ndcube_2d_ln_lt): | ||
| cube = ndcube_2d_ln_lt | ||
| frame = astropy.wcs.utils.wcs_to_celestial_frame(cube.wcs) | ||
| lower_corner = SkyCoord(Tx=[0359.99667], Ty=[-0.0011111111], unit="deg", frame=frame) |
There was a problem hiding this comment.
why are these coordinates so specific?
There was a problem hiding this comment.
I copied them from another test. Can't remember why that test had them so specific
That rings a bell but couldn't see the issue at a glance |
ndcube/utils/cube.py
Outdated
| for point in points: | ||
| # Sanitize input format | ||
| # Make point a tuple if given as a single high level coord object valid for this WCS. | ||
| if isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): |
There was a problem hiding this comment.
activates APE-14 lawyer mode
This is technically invalid if wcs.seralized_classes is True, as the first element of world_axis_object_classes will be a string. I've never actually seen a WCS in the wild which uses seralized_classes so we could just throw an error here if it's True?
There was a problem hiding this comment.
Would simply not supporting this sanitization this check when wcs.seralized_classes is True be sufficient? i.e.
| if isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): | |
| if not isinstance(point, (tuple, list)) and not wcs.seralized_classes and isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): |
ndcube/utils/cube.py
Outdated
| # If point is a length-1 object, convert it to scalar. | ||
| point = tuple(p.squeeze() if hasattr(p, "squeeze") else p for p in point) |
There was a problem hiding this comment.
This is unrelated to the tuple thing right?
There was a problem hiding this comment.
Yes. Here, we can assume that point is a tuple or list. So this line is about converting length-1 objects to scalar ones.
There was a problem hiding this comment.
I'm a little lost why we need to do this, can't we handle multi-dimensional inputs?
ndcube/utils/cube.py
Outdated
| for point in points: | ||
| # Sanitize input format | ||
| # Make point a tuple if given as a single high level coord object valid for this WCS. | ||
| if isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): |
There was a problem hiding this comment.
| if isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): | |
| if not isinstance(point, tuple) and isinstance(point, tuple(v[0] for v in wcs.world_axis_object_classes.values())): |
|
I think this got messed up before you merged it, the diff is wrong. |
|
REVERT |
|
@DanRyanIrish What's this PR actually done? I'm now very confused and slightly annoyed it got merged after becoming something else? |
|
The fix is now here and this is all that's actually required. Much simpler. |
4 participants
PR Description
Resolves Issue #861