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

Jump to bottom

Reimplement NDCube.crop to "crop by points" and Extend Support to Crop by ExtraCoords #472

Merged
merged 59 commits into from
Oct 19, 2021
Merged

Reimplement NDCube.crop to "crop by points" and Extend Support to Crop by ExtraCoords #472

merged 59 commits into from
Oct 19, 2021

Conversation

DanRyanIrish
Copy link
Member

@DanRyanIrish DanRyanIrish commented Sep 17, 2021 •
edited
Loading

Description

This PR reimplements NDCube.crop (and NDCube.crop_by_values) to use the "crop by points" algorithm. Instead of taking lower and upper bounds on the world axes and calculating the corners of the bounding box in pixel space, this PR makes NDCube.crop return the smallest cube in pixel space that includes the user-input points described using world coords. It is not assumed that the entered points are corners. This is degenerate with the bounding box implementation when the world and pixel axes are aligned. However, when the coordinate systems are rotated, the returned cube can be much smaller.

This PR also extends the crop functionality to enable cropping by extra coords. As a result, it introduces a new property, ExtraCoords.cube_wcs, which returns a WCS of the extra coords that describes the cube itself. If not all array axes are associated with an extra coord, dummy world axes are added.

This PR is a follow on attempt to #452

Fixes #345 and #476

Requires #482 to be merged.

DanRyanIrish and others added 30 commits August 4, 2021 11:20
@DanRyanIrish
1st attempt at supporting cropping by extra coords in 2.0.
450062d
@DanRyanIrish
2nd attempt to make NDCube.crop to support extra_coords.
d9a7fcd
@DanRyanIrish
Fix bug in CompoundWCS world_to_pixel_values inputs check.
5cf8d74 
CompoundWCS's check as to whether input world values resulted in
compatible pixel values when world dimensions share pixel dimensions was
not appropriate for all mappings.  This commit introduces an alternative
implementation to overcome this.
@DanRyanIrish
Removes commented out code.
2cdebe3
@DanRyanIrish
Fixes codestyle.
aa8cea2
@DanRyanIrish
Add test for NDCube.crop with ExtraCoords.
25a626f
@DanRyanIrish
Add test for cropping NDCube by values with ExtraCroods.
4153c6a
@DanRyanIrish
Add sunpy#452 changelogs.
117a4f2
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
4e73c70
@DanRyanIrish @Cadair
Apply suggestions from code review
8d11088 
Co-authored-by: Stuart Mumford <stuart@cadair.com>
@DanRyanIrish
Add property to ExtraCoords that returns a WCS describing whole NDCube.
0b60e2d
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
d312f8a
@DanRyanIrish
Fix bug in ExtraCoords._cube_array_axes_without_extra_coords.
df00ed2
@DanRyanIrish
Add feature changelog for sunpy#452.
3e66c38
@DanRyanIrish
Merge branch 'crop_by_ec' of https://github.com/DanRyanIrish/ndcube i…
c4f53c9 
…nto crop_by_ec
@DanRyanIrish
Enable NDCube crop by extra_coords when all axes have an associated e…
70ee175 
…xtra coord.
@DanRyanIrish
Enable NDCube to crop by extra coords that share a pixel axis.
572f2db
@DanRyanIrish
Move calculation of slice item in NDCube.crop to utils.
e60158e 
This enables the item to be calculated without slicing the cube which
can be useful, for example when trying to mimic crop's behaviour for
NDCubeSequence.
@DanRyanIrish
Refactor crop bounding box calculation.
3185899 
Refactor avoids errors caused by creating corners by permutating values
of world axes that share the same pixel axis.  When this is the case, it
leads to an undefined location on the pixel grid.

Tests not yet passing.
@DanRyanIrish
Fix bug stripping and reattaching units in NDCube crop bounding box c…
c6c5dd9 
…alculation.
@DanRyanIrish
Codestyle fixes.
9c7bdf8
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
a2aa62e
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
c3e2cbf
@DanRyanIrish
Special-case defining NDCube crop corners when number fo world and pi…
16b92ba 
…xel dims is the same.

This leads to a simpler code path in most cases.
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
931334d
@DanRyanIrish
Fix codestyle.
65b0899
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_by_ec
e7390f5
@DanRyanIrish
Merge branch 'main' of https://github.com/sunpy/ndcube into crop_utils
5ce92cb
@DanRyanIrish
Add test for NDCube._bounding_box_to_points.
0c056e8
@DanRyanIrish
Remove special case in NDCube._bounding_box_to_points when number of …
5fc62cd 
…world and pixel axes the same.

Closer inspection reveals the simpler implementation for this case is
not always valid.
@DanRyanIrish DanRyanIrish marked this pull request as ready for review October 17, 2021 11:02
@DanRyanIrish
Copy link
Member Author

@Cadair, this PR now has all tests passing and is ready for review.

Outstanding Questions

  1. Should crop reduce the dimensionality if the cropped cube has length-1 along an array axis? This implementation makes it much easier to make crop do this if we want. It seems @nabobalis 's experience/intuition is that it should.
  2. As it is, this PR allows some world coords to be entered as None. But if this is the case, then all world coords corresponding to the same array axis must be None. In most cases, this is intuitive in practice. However, if we have a single array axis corresponding to multiple independent 1-D world coords, then if the value for one coord is provided, the user must provide all coords despite the fact that in principle, the value of a single coord is enough to uniquely identify the array index. I suggest this level of optimisation is beyond the scope of this PR and of 2.0.
  3. My understanding is that we have agreed that these method names (crop and crop_by_values) are correct for the current behaviour (i.e. the crop by points algorithm.) If we want to revisit that we should do it before merging. My vote is we stick with the names and implementations as this PR currently has them.

@DanRyanIrish DanRyanIrish changed the title WIP: Reimplement NDCube.crop to "crop by points" and Extend Support to Crop by ExtraCoords Reimplement NDCube.crop to "crop by points" and Extend Support to Crop by ExtraCoords Oct 17, 2021
@DanRyanIrish
Copy link
Member Author

Failing tests should be fixes once #482 is merged.

@nabobalis
Copy link
Contributor

  1. Should crop reduce the dimensionality if the cropped cube has length-1 along an array axis? This implementation makes it much easier to make crop do this if we want. It seems @nabobalis 's experience/intuition is that it should.

If you don't want to do it here, what about in sunraster instead?

@DanRyanIrish
Copy link
Member Author

DanRyanIrish commented Oct 17, 2021 •
edited
Loading

If you don't want to do it here, what about in sunraster instead?

I'm implementing it here.

@Cadair
Copy link
Member

Cadair commented Oct 19, 2021 •
edited
Loading

  1. Should crop reduce the dimensionality if the cropped cube has length-1 along an array axis? This implementation makes it much easier to make crop do this if we want. It seems @nabobalis 's experience/intuition is that it should.

I think this makes a lot of sense if N points is one along a given axis?

  1. As it is, this PR allows some world coords to be entered as None. But if this is the case, then all world coords corresponding to the same array axis must be None. In most cases, this is intuitive in practice. However, if we have a single array axis corresponding to multiple independent 1-D world coords, then if the value for one coord is provided, the user must provide all coords despite the fact that in principle, the value of a single coord is enough to uniquely identify the array index. I suggest this level of optimisation is beyond the scope of this PR and of 2.0.

👍 I am happy for this to be a feature request and not hold up 2.0 on it.

  1. My understanding is that we have agreed that these method names (crop and crop_by_values) are correct for the current behaviour (i.e. the crop by points algorithm.) If we want to revisit that we should do it before merging. My vote is we stick with the names and implementations as this PR currently has them.

👍 this was my recollection also as crop didn't exist in 1.x anyway.

@Cadair Cadair self-requested a review October 19, 2021 10:40
Cadair
Cadair approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

A few minor points.

ndcube/conftest.py Outdated Show resolved Hide resolved
ndcube/conftest.py Outdated Show resolved Hide resolved
ndcube/extra_coords/extra_coords.py Outdated Show resolved Hide resolved
ndcube/tests/test_ndcube.py Show resolved Hide resolved
ndcube/utils/cube.py Show resolved Hide resolved
ndcube/utils/cube.py Outdated Show resolved Hide resolved
@DanRyanIrish
Copy link
Member Author

Changes made. Tests passing. Merging!

@DanRyanIrish DanRyanIrish merged commit 73b4589 into sunpy:main Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cadair Cadair Cadair approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

Does crop actually work with just extra_coords?
4 participants
@DanRyanIrish @pep8speaks @nabobalis @Cadair