Bug fixes in WCSAxes.plot_coord #8927

Cadair · 2019-06-27T14:46:51Z

This fixes two bugs in plot_coord:

It more correctly uses .spherical rather than .data to get lon and lat.
It does the frame transform on the input coordinate frame rather than relying on the matplotlib transforms later.

The latter is a fix because the matplotlib transforms are only ever two dimensional, and it should not be assumed that the input coordinate to plot_coord is two dimensional, as it can be in any frame transformable to the native frame of the plot.

I can't really think of a way to test this with just the astropy frames, if anyone has any suggestions of a transformation which would work in 3D but not 2D in astropy frames that would be great.

ping @ayshih

pllim · 2019-06-27T15:05:20Z

Is this addressing any existing Astropy issue(s)? Also please add change log and tests. Thanks!

CHANGES.rst

astrofrog

Just a couple of comments below - thanks!

astrofrog · 2019-06-28T08:58:32Z

astropy/visualization/wcsaxes/core.py

@@ -299,7 +304,7 @@ def plot_coord(self, *args, **kwargs):
                raise TypeError("The 'transform' keyword argument is not allowed,"
                                " as it is automatically determined by the input coordinate frame.")

-            transform = self.get_transform(frame0)
+            transform = self.get_transform(native_frame)


I think you can maybe just do 'world' here

I did that at one point but this seemed easier to read. Is there a good reason for one or the other?

astrofrog · 2019-06-28T08:59:38Z

astropy/visualization/wcsaxes/core.py

                elif coord.coord_type == 'latitude':
-                    plot_data.append(frame0.data.lat.to_value(coord.coord_unit))
+                    plot_data.append(frame0.spherical.lat.to_value(u.deg))


I wonder whether it would make sense to only access spherical if the current representation is not spherical or unitspherical, for performance? Or at least pre-compute frame0.spherical to avoid computing it twice.

I think that frames cache their different representations anyway, so this should be basically free on both counts.

astrofrog · 2019-06-28T09:15:22Z

Possible regression test (fails pretty badly with master):

from astropy import units as u
from astropy.wcs import WCS
from astropy.coordinates import SkyCoord
from astropy.utils.data import get_pkg_data_filename
from astropy.io import fits

import matplotlib.pyplot as plt

filename = get_pkg_data_filename('galactic_center/gc_msx_e.fits')
wcs = WCS(filename)
data = fits.getdata(filename)

coord = SkyCoord(0 * u.kpc, 0 * u.kpc, 0 * u.kpc, frame='galactocentric')

fig = plt.figure()
ax = fig.add_subplot(1, 1, 1, projection=wcs)
ax.imshow(data)
ax.plot_coord(coord, 'ro')
fig.savefig('galcen.png')

The marker should be at the center of the image.

Cadair · 2019-07-04T14:09:59Z

@astrofrog Not sure what the procedure is for the image tests?

astrofrog · 2019-07-04T15:06:50Z

The image test docs are here: http://docs.astropy.org/en/stable/development/testguide.html#image-tests-with-pytest-mpl

However, can't you just check the pixel coordinates of the resulting marker without doing a full image test? (which seems overkill here)

Cadair · 2019-07-05T10:12:56Z

@astrofrog I changed the test. Can you take a look to check I am comparing it to the right thing?

Also I changed the isort config because I needed to use isort.

astrofrog · 2019-07-05T13:36:11Z

astropy/visualization/wcsaxes/tests/test_display_world_coordinates.py

+        ax.imshow(data)
+        point, = ax.plot_coord(coord, 'ro')
+
+        np.testing.assert_allclose(point.get_xydata()[0], [0, 0], atol=1e-4)


I'm surprised that this would be at [0,0], it should be near the center of the image.

yeah this one confused me as well. is get_xydata in pixels?

Looks like it should be 😕

Ah no I think it might be the data before applying the transform. So to some extent this test is correct but I'd still be happier if we can figure out how to get the transformed data.

You might need point.get_transform().transform(point.get_xydata())

That didn't work either, I can't see any other sensible solution other than this here 😕

This means the transform can be 3D without having to interact with the mpl transforms infrastructure.

astrofrog

Looks great, and thanks for fixing the isort config!

Bug fixes in WCSAxes.plot_coord

bsipocz · 2019-10-07T18:50:20Z

astropy/visualization/wcsaxes/core.py

@@ -289,12 +290,16 @@ def plot_coord(self, *args, **kwargs):
            if isinstance(frame0, SkyCoord):
                frame0 = frame0.frame

+            native_frame = self._transform_pixel2world.frame_out


This doesn't work on 3.2.2:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <astropy.visualization.wcsaxes.core.WCSAxes object at 0x13297d210> args = (<SkyCoord (ICRS): (ra, dec) in deg (359.76045223, 0.26876217)>, 'o') kwargs = {'transform': <matplotlib.transforms.CompositeGenericTransform object at 0x131f72190>} frame0 = <ICRS Coordinate: (ra, dec) in deg (359.76045223, 0.26876217)> def plot_coord(self, *args, **kwargs): """ Plot `~astropy.coordinates.SkyCoord` or `~astropy.coordinates.BaseCoordinateFrame` objects onto the axes. The first argument to :meth:`~astropy.visualization.wcsaxes.WCSAxes.plot_coord` should be a coordinate, which will then be converted to the first two parameters to `matplotlib.axes.Axes.plot`. All other arguments are the same as `matplotlib.axes.Axes.plot`. If not specified a ``transform`` keyword argument will be created based on the coordinate. Parameters ---------- coordinate : `~astropy.coordinates.SkyCoord` or `~astropy.coordinates.BaseCoordinateFrame` The coordinate object to plot on the axes. This is converted to the first two arguments to `matplotlib.axes.Axes.plot`. See Also -------- matplotlib.axes.Axes.plot : This method is called from this function with all arguments passed to it. """ if isinstance(args[0], (SkyCoord, BaseCoordinateFrame)): # Extract the frame from the first argument. frame0 = args[0] if isinstance(frame0, SkyCoord): frame0 = frame0.frame > native_frame = self._transform_pixel2world.frame_out E AttributeError: 'WCSAxes' object has no attribute '_transform_pixel2world' astropy/visualization/wcsaxes/core.py:295: AttributeError

(relies on the reorganization from #8874)

Cadair mentioned this pull request Jun 27, 2019

Added plot for venus transit for coordinate section sunpy/sunpy-1.0-paper#16

Closed

pllim added visualization.wcsaxes Bug labels Jun 27, 2019

pllim added this to the v3.2.2 milestone Jun 27, 2019

pllim requested a review from astrofrog June 27, 2019 15:04

Cadair force-pushed the fix_plotcoord branch from 45e4f42 to 4a5792b Compare June 27, 2019 15:28

bsipocz reviewed Jun 27, 2019

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

astrofrog reviewed Jun 28, 2019

View reviewed changes

Cadair force-pushed the fix_plotcoord branch from 4a5792b to 107ce60 Compare July 4, 2019 14:04

astrofrog reviewed Jul 5, 2019

View reviewed changes

Cadair added 8 commits September 23, 2019 11:04

use spherical to get the data rather than .data

e3b3acd

Do the transform to the native frame plot_coord.

4e35bc4

This means the transform can be 3D without having to interact with the mpl transforms infrastructure.

@astrofrog says it assumes degrees

ff83e68

Add a changelog

4dc8e0f

Add a regression test

a1e9a0f

wrap lines in changelog

ab04c0d

Change test for plot_coord

b1d225b

fixup isort config

7d50886

Cadair force-pushed the fix_plotcoord branch 2 times, most recently from 7532a66 to 4c2012e Compare September 23, 2019 10:54

Add tests for plot_coord bugs

f9ad9c0

Cadair force-pushed the fix_plotcoord branch from 4c2012e to f9ad9c0 Compare September 23, 2019 10:56

bsipocz requested a review from astrofrog September 27, 2019 01:14

astrofrog approved these changes Oct 4, 2019

View reviewed changes

astrofrog merged commit 8e5831a into astropy:master Oct 4, 2019

bsipocz pushed a commit that referenced this pull request Oct 5, 2019

Merge pull request #8927 from Cadair/fix_plotcoord

a82755e

Bug fixes in WCSAxes.plot_coord

bsipocz reviewed Oct 7, 2019

View reviewed changes

bsipocz modified the milestones: v3.2.2, v4.0 Oct 7, 2019

ayshih mentioned this pull request Oct 28, 2020

Removed unnecessary transformations of coordinates prior to using plot_coord() sunpy/sunpy#4609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes in WCSAxes.plot_coord #8927

Bug fixes in WCSAxes.plot_coord #8927

Cadair commented Jun 27, 2019

pllim commented Jun 27, 2019

astrofrog left a comment

astrofrog Jun 28, 2019

Cadair Jun 28, 2019

astrofrog Jun 28, 2019

Cadair Jun 28, 2019

astrofrog commented Jun 28, 2019 •

edited

Cadair commented Jul 4, 2019

astrofrog commented Jul 4, 2019

Cadair commented Jul 5, 2019

astrofrog Jul 5, 2019

Cadair Jul 5, 2019

astrofrog Jul 5, 2019

astrofrog Jul 5, 2019

astrofrog Jul 5, 2019

Cadair Sep 23, 2019

astrofrog left a comment

bsipocz Oct 7, 2019

bsipocz Oct 7, 2019

Bug fixes in WCSAxes.plot_coord #8927

Bug fixes in WCSAxes.plot_coord #8927

Conversation

Cadair commented Jun 27, 2019

pllim commented Jun 27, 2019

astrofrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Jun 28, 2019 • edited

Cadair commented Jul 4, 2019

astrofrog commented Jul 4, 2019

Cadair commented Jul 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Jun 28, 2019 •

edited