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.

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

Adds ValueError to mapbase.submap() when Helioprojection coordinate contains NaN values. #7543

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

grahamasam
Copy link

@grahamasam grahamasam commented Apr 1, 2024

This pull request produces a value error when Helioprojection coordinates that have NaN distance values are input as bottom_left or top_right to the submap() function.

Fixes #5598

Additional Info:
I'm unsure if this solution fully addresses the common user issue presented in #5598. A testcase that mimics one of these specific user issues could be useful to determine what other situations should result in the ValueError. Any suggestions would be greatly appreciated!

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@nabobalis nabobalis added this to the 6.0.0 milestone May 8, 2024
@nabobalis nabobalis force-pushed the submap_nan branch 2 times, most recently from 801e624 to db50a75 Compare May 26, 2024 04:01
@nabobalis nabobalis marked this pull request as ready for review May 26, 2024 04:05
@nabobalis nabobalis requested a review from a team as a code owner May 26, 2024 04:05
@nabobalis nabobalis added the Needs Review Needs reviews before merge label May 26, 2024
msg = (
"The {quadrant} input contains NaN values. "
"It is possible the coordinates are off-disk. "
"Consider using Helioprojective.assume_spherical_screen"
Copy link
Member

Choose a reason for hiding this comment

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

If we merge #7115, this should change to SphericalScreen

Comment on lines 1998 to 2000
"The {quadrant} input contains NaN values. "
"It is possible the coordinates are off-disk. "
"Consider using Helioprojective.assume_spherical_screen"
Copy link
Member

Choose a reason for hiding this comment

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

This reads more like a warning than an exception. My issue with it is it doesn't really say why an exception is being raised. Why am I not being allowed to crop my map when one of my coordinates is off disk?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why am I not allowed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added more text to the message, see if you like it.

@@ -0,0 +1 @@
Updated :meth:`sunpy.map.Map.submap` to check if it is about to work on locations with NaNs now errors and informs the user that they likely want to use :meth:`~sunpy.coordinates.Helioprojective.assume_spherical_screen` so that the off-disk 2D coordinate can be converted to a 3D coordinate.
Copy link
Member

Choose a reason for hiding this comment

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

Same as below. Should be updated to use SphericalScreen once that PR is merged.

@nabobalis nabobalis added map Affects the map submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Jun 12, 2024
@nabobalis
Copy link
Contributor

Copy link
Member

@ayshih ayshih left a comment

Choose a reason for hiding this comment

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

Oops, I guess I haven't looked at this PR before. As guessed by the author, this PR doesn't actually address the common use case of #5598 because the implementation is at the "wrong" place in the code. (The included unit tests do not reflect common usage.) I'll try to provide better guidance soon if someone else doesn't do so first.

@ayshih
Copy link
Member

ayshih commented Jun 14, 2024

Here's an example of the common use case:

>>> import astropy.units as u
>>> from astropy.coordinates import SkyCoord

>>> import sunpy.map
>>> from sunpy.data.sample import AIA_171_IMAGE

>>> aiamap = sunpy.map.Map(AIA_171_IMAGE)

The following submap that includes off-disk pixels works fine because it is in the native coordinate frame, so 2D coordinates do not need to be promoted:

>>> coord_native = SkyCoord(0*u.arcsec, 0*u.arcsec, frame=aiamap.coordinate_frame)
>>> aiamap.submap(coord_native, width=1000*u.arcsec, height=1000*u.arcsec)

The following, very similar submap fails because the observer is slightly different, so 2D coordinates need to be transformed to the native coordinate frame, and thus the off-disk 2D corners get a NaN distance when internally promoted.

>>> coord_other = SkyCoord(0*u.arcsec, 0*u.arcsec, frame='helioprojective', observer='earth', obstime=aiamap.date)
>>> aiamap.submap(coord_other, width=1000*u.arcsec, height=1000*u.arcsec)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: cannot convert float NaN to integer

The check needs to be on the output of self._parse_submap_input() right here:

sunpy/sunpy/map/mapbase.py

Lines 1912 to 1913 in e7536b1

pixel_corners = u.Quantity(self._parse_submap_input(
bottom_left, top_right, width, height)).T

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule Needs Review Needs reviews before merge No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a helpful error message to GenericMap.submap() for helioprojective-related NaNs
4 participants