Skip to content

[map branch] Adopt GenericMap.submap to use NDCube.crop underneath #7605

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
Draft
wants to merge 5 commits into
base: map_ndcube_migration
Choose a base branch
from
Draft

[map branch] Adopt GenericMap.submap to use NDCube.crop underneath #7605

wants to merge 5 commits into from
+23 −62

Conversation

hayesla
Copy link
Member

@hayesla hayesla commented Apr 26, 2024

This PR is add NDCube.crop within GenericMap.submap

@hayesla hayesla requested a review from a team as a code owner April 26, 2024 10:40
@wtbarnes wtbarnes changed the title Adopt GenericMap.submap to use NDCube.crop underneath [map branch] Adopt GenericMap.submap to use NDCube.crop underneath Apr 26, 2024
@nabobalis nabobalis added the No Changelog Entry Needed Skip all changelog checks. label May 8, 2024
@hayesla
Copy link
Member Author

hayesla commented May 27, 2024

ok, so I think this is mostly working. Two issues we need to consider:

  1. How we deal with slicing to a single pixel, which can happen of course when people use SkyCoord etc where this may not be as obvious as this example. This works with the current implementation of submap in sunpy
>>> aia_map = sunpy.map.Map(sunpy.data.sample.AIA_171_IMAGE)
>>> aia_map.submap([0, 0]*u.pixel, top_right=[0, 0]*u.pixel)

ValueError: Input points causes cube to be cropped to a single pixel. This is not supported.
  1. How we deal with reducing the number of dimensions. This can of course be dealt with in the submap function, just interested in peoples thoughts.
>>> aia_map.submap([0, 0]*u.pixel, top_right=[0, 2]*u.pixel)

TypeError: It is not possible to slice a map with an integer as it will reduce the number of data dimensions by one.
In order to apply the same slice without dropping a dimension do mymap[:2, 1:2].

@hayesla
Copy link
Member Author

hayesla commented May 27, 2024

just fyi - thats why I've commented out these tests. I'll adjust when we figure it out

@hayesla hayesla mentioned this pull request May 29, 2024
Closed
@hayesla
Copy link
Member Author

hayesla commented Jun 19, 2024 •
edited
Loading

linking this with the NDCube issue sunpy/ndcube#714 so we dont forget again

@DanRyanIrish
Copy link
Member

The decision taken on the sunpy call is to solve this issue by introducing a keepdims kwarg to NDCubeBase.crop and then setting this to True inside Map.submap. See sunpy/ndcube#714 for more details on how to achieve this.

@DanRyanIrish
Copy link
Member

@hayesla : sunpy/ndcube#714 has been merged. So you can now finish this PR by using the keepdims kwarg of NDCube.crop.

@DanRyanIrish
Copy link
Member

Hi @nabobalis . Have you taken over this PR?

@nabobalis
Copy link
Member

Hi @nabobalis . Have you taken over this PR?

No idea.

nabobalis
nabobalis reviewed Aug 21, 2024
View reviewed changes
@nabobalis nabobalis force-pushed the submap_to_crop branch 4 times, most recently from b59ac2f to b69ad5f Compare August 21, 2024 00:51
nabobalis
nabobalis reviewed Aug 21, 2024
View reviewed changes
sunpy/map/mapbase.py
Comment on lines +1174 to +1186
# TODO HACK FOR NOW
import warnings
with warnings.catch_warnings():
warnings.filterwarnings("ignore")
if np.any(np.isnan([self.wcs.world_to_pixel(world_corner) for world_corner in world_corners])):
raise ValueError(msg)
Copy link
Member

Choose a reason for hiding this comment

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

This needs revising but I dont got any good ideas.

@nabobalis nabobalis requested a review from a team as a code owner August 21, 2024 01:12
nabobalis
nabobalis reviewed Aug 21, 2024
View reviewed changes
sunpy/map/tests/test_mapbase.py
Comment on lines +733 to +711
[([0, 0] * u.pix, [0.5, 0.5] * u.pix), np.array([[0, 1],
[ 9, 10]])],
Copy link
Member

Choose a reason for hiding this comment

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

The behavior changed here. Do we want this?

@nabobalis nabobalis marked this pull request as draft October 23, 2024 23:14
@github-actions GitHub Actions
Copy link
Contributor

Hello 👋, Thanks for your contribution to sunpy!
I have marked this pull request as stale because there hasn't had any activity in five months. If you are still working on this, or if it's waiting on a maintainer to look at it then please let us know and we will keep it open. Please add a comment with: @sunpy/sunpy-developers to get someone's attention.
If nobody comments on this pull request for another month, it will be closed.

@github-actions github-actions bot added the Stale The bot will close this PR after 6 months (if enabled on this repo). label Mar 23, 2025
@github-actions GitHub Actions
Copy link
Contributor

Hello again 👋, We want to thank you again for your contribution to sunpy!
This pull request has had no activity since my last reminder, so I am going to close it. If at any time you want to come back to this please feel free to reopen it! If you want to discuss this, please add a comment with: @sunpy/sunpy-developers and someone will get back to you soon.

@github-actions github-actions bot closed this Apr 22, 2025
@Cadair Cadair reopened this Apr 22, 2025
@github-actions github-actions bot removed the Stale The bot will close this PR after 6 months (if enabled on this repo). label Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabobalis nabobalis nabobalis left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
No Changelog Entry Needed Skip all changelog checks.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hayesla @DanRyanIrish @nabobalis @Cadair @CyclingNinja