Repository: sunpy/sunpy

Issue:
Add a helpful warning message when converting a 2D `Helioprojective` coordinate will return all NaNs
If a 2D `Helioprojective` coordinate is off-disk, the conversion from 2D to 3D that is required for any non-trivial coordinate transformation will return all NaNs, unless something like `Helioprojective.assume_spherical_screen()` is used.  This is a recurring source of confusion for our users, particularly because they also sometimes fail to realize that they are asking for a non-trivial coordinate transformation.

We can add a helpful warning message when `Helioprojective.make_3d()` is going to turn a coordinate array that is not all NaNs into a coordinate array that is all NaNs*.  It would inform the user that a non-trivial transformation may have been requested and that a user may wish to use `Helioprojective.assume_spherical_screen()`.

\* We would not emit the warning if only some of the elements of a coordinate array turn into NaNs, because this happens all the time in mass transformations (e.g., with `reproject`).

See #5598 for a somewhat related issue

Discussion:
pep8speaks: Hello @thewindsofwinter! Thanks for opening this PR. 

* In the file [`sunpy/coordinates/frames.py`](https://github.com/sunpy/sunpy/blob/6f10bd5a52430a8b3f45cf157ce6dc76ad0d3580/sunpy/coordinates/frames.py):

> [Line 593:37](https://github.com/sunpy/sunpy/blob/6f10bd5a52430a8b3f45cf157ce6dc76ad0d3580/sunpy/coordinates/frames.py#L593): [E127](https://duckduckgo.com/?q=pep8%20E127) continuation line over-indented for visual indent

Do see the [Hitchhiker's guide to code style](https://goo.gl/hqbW4r)
nabobalis: > * [ ]  Testing: I was unable to test the code as tox kept throwing errors on my laptop. Looks relatively straightforward, though, so hopefully it will work (and others hopefully will have better luck testing if it doesn't.

What errors?

> Thoughts on including a checklist for contributers in pull request template? That way we can try to ensure code follows standards set forth in the guidelines. Something like [this](https://github.com/gitkraken/vscode-gitlens/blob/main/.github/PULL_REQUEST_TEMPLATE.md?rgh-link-date=2022-01-15T05%3A30%3A53Z) but of course customized for sunpy. If needed I can create an issue, make quick edits to this template, and send in another PR after this.

We can look into adding something similar, yes. 


thewindsofwinter: > > * [ ]   Testing: I was unable to test the code as tox kept throwing errors on my laptop. Looks relatively straightforward, though, so hopefully it will work (and others hopefully will have better luck testing if it doesn't.
> 
> What errors?

Import errors from the environment itself, and I'm too worried to edit it since this was something conda itself generated

  File "$HOME\.conda\envs\sunpy-dev\lib\ctypes\__init__.py", line 8, in <module>
    from _ctypes import Union, Structure, Array
ImportError: DLL load failed while importing _ctypes: The specified module could not be found.
nabobalis: >  File "$HOME\.conda\envs\sunpy-dev\lib\ctypes\__init__.py", line 8, in <module>
>    from _ctypes import Union, Structure, Array
> ImportError: DLL load failed while importing _ctypes: The specified module could not be found.

I can't say I've ever seen this error but can you create a new Python environment or reinstall conda? 
thewindsofwinter: > > File "$HOME.conda\envs\sunpy-dev\lib\ctypes__init__.py", line 8, in 
> > from _ctypes import Union, Structure, Array
> > ImportError: DLL load failed while importing _ctypes: The specified module could not be found.
> 
> I can't say I've ever seen this error but can you create a new Python environment or reinstall conda?

I'll try again then!
ayshih: Convenient: the unit test already exists!  As @dstansby said, you should add a `pytest.warns()` to the the test `test_hpc_distance_off_limb`.  For the test `test_skycoord_hpc`, it might make sense to instead add to the existing warning filtering in that test because only 1 of the 7 parameterized arguments actually emits the warning.
nabobalis: Looks like a test is still failing? 
thewindsofwinter: Yes, sorry, I've been busy as of late. I'm going to fix this once I get some spare time (unless someone else does before then)
nabobalis: > Yes, sorry, I've been busy as of late. I'm going to fix this once I get some spare time (unless someone else does before then)

That is no problem, real life is more important than PRs

Patch:
diff --git a/changelog/5817.feature.rst b/changelog/5817.feature.rst
new file mode 100644
index 000000000..9ac118219
--- /dev/null
+++ b/changelog/5817.feature.rst
@@ -0,0 +1,1 @@
+Added a helpful warning message when converting a 2D Helioprojective coordinate will return all NaNs.
diff --git a/sunpy/coordinates/frames.py b/sunpy/coordinates/frames.py
index 54da0ae96..b24d8237f 100644
--- a/sunpy/coordinates/frames.py
+++ b/sunpy/coordinates/frames.py
@@ -4,6 +4,8 @@ Common solar physics coordinate systems.
 This submodule implements various solar physics coordinate frames for use with
 the `astropy.coordinates` module.
 """
+import re
+import traceback
 from contextlib import contextmanager
 
 import numpy as np
@@ -587,6 +589,18 @@ class Helioprojective(SunPyBaseCoordinateFrame):
 
             d = np.fmin(d, dd) if self._spherical_screen['only_off_disk'] else dd
 
+        # This warning can be triggered in specific draw calls when plt.show() is called
+        # we can not easily prevent this, so we check the specific function is being called
+        # within the stack trace.
+        stack_trace = traceback.format_stack()
+        matching_string = 'wcsaxes.*_draw_grid'
+        bypass = any([re.search(matching_string, string) for string in stack_trace])
+        if not bypass and np.all(np.isnan(d)) and np.any(np.isfinite(cos_alpha)):
+            warn_user("The conversion of these 2D helioprojective coordinates to 3D is all NaNs "
+                      "because off-disk coordinates need an additional assumption to be mapped to "
+                      "calculate distance from the observer. Consider using the context manager "
+                      "`Helioprojective.assume_spherical_screen()`.")
+
         return self.realize_frame(SphericalRepresentation(lon=lon,
                                                           lat=lat,
                                                           distance=d))
diff --git a/sunpy/map/mapbase.py b/sunpy/map/mapbase.py
index 0271073d7..6614c9eb0 100644
--- a/sunpy/map/mapbase.py
+++ b/sunpy/map/mapbase.py
@@ -2154,7 +2154,6 @@ class GenericMap(NDData):
         if not axes:
             axes = wcsaxes_compat.gca_wcs(self.wcs)
         is_wcsaxes = wcsaxes_compat.is_wcsaxes(axes)
-
         if is_wcsaxes:
             # TODO: supply custom limb radius
             return sunpy.visualization.limb.draw_limb(
@@ -2165,7 +2164,6 @@ class GenericMap(NDData):
                             "and will be removed in sunpy 4.1.")
             # If not WCSAxes use a Circle
             c_kw.setdefault('radius', self.rsun_obs.value)
-
             circ = patches.Circle([0, 0], **c_kw)
             axes.add_artist(circ)
             return circ, None
@@ -2364,7 +2362,6 @@ class GenericMap(NDData):
 
         if draw_limb:
             self.draw_limb(axes=axes)
-
         if draw_grid:
             if grid_spacing is None:
                 self.draw_grid(axes=axes)


Test patch:
diff --git a/sunpy/coordinates/tests/test_frames.py b/sunpy/coordinates/tests/test_frames.py
index 02dd32edf..18a959aec 100644
--- a/sunpy/coordinates/tests/test_frames.py
+++ b/sunpy/coordinates/tests/test_frames.py
@@ -176,14 +176,13 @@ def test_hpc_distance_off_limb():
     assert hpc1.Tx == 1500 * u.arcsec
     assert hpc1.Ty == 0 * u.arcsec
 
-    hpc2 = hpc1.make_3d()
-
-    assert isinstance(hpc2._data, SphericalRepresentation)
-
-    # Check the attrs are correct
-    assert hpc2.Tx == 1500 * u.arcsec
-    assert hpc2.Ty == 0 * u.arcsec
-    assert_quantity_allclose(hpc2.distance, u.Quantity(np.nan, u.km))
+    with pytest.raises(SunpyUserWarning, match="is all NaNs"):
+        hpc2 = hpc1.make_3d()
+        assert isinstance(hpc2._data, SphericalRepresentation)
+        # Check the attrs are correct
+        assert hpc2.Tx == 1500 * u.arcsec
+        assert hpc2.Ty == 0 * u.arcsec
+        assert_quantity_allclose(hpc2.distance, u.Quantity(np.nan, u.km))
 
 
 def test_hpc_distance_3D():
@@ -414,17 +413,14 @@ def test_skycoord_hpc(args, kwargs):
     Test that when instantiating a HPC frame with SkyCoord that make_3d
     still works.
     """
-
     sc = SkyCoord(*args, **kwargs, frame="helioprojective",
                   observer='earth', obstime="2011-01-01T00:00:00")
     # Test the transform to HGS because it will force a `make_3d` call.
-    #
-    # astropy emits some warnings here because of invalid NaN comparisons,
-    # which will be removed in a future astropy release
-    # (see https://github.com/astropy/astropy/pull/9843), so filter the warnings out.
+    # Only 1 of the 7 parameterized arguments actually emits the warning
     with warnings.catch_warnings():
-        warnings.filterwarnings("ignore", message='invalid value encountered',
-                                category=RuntimeWarning)
+        warnings.filterwarnings("ignore", message=(
+            'The conversion of these 2D helioprojective coordinates to '
+        ), category=SunpyUserWarning)
         hgs = sc.transform_to("heliographic_stonyhurst")
 
     assert isinstance(hgs.frame, HeliographicStonyhurst)