Fix export to Python script to correctly use Coordinates or WCS in reference_data

[dhomeier]

Description

This fixes an issue I found when trying to use the Save Python script function on a viewer using a dataset with a standard (Astropy) WCS, such as the example w5.fits data. The script thus created failed with an

AttributeError: 'astropy.wcs.Wcsprm' object has no attribute 'pixel_n_dim'

on ax.reset_wcs(slices=['x', 'y', 0], wcs=ref_data.coords.wcs) as ref_data.coords itself is already the WCS.

The fix, to test for that case first in the exporter, seems quite straightforward.
I've parametrised some test to run on a dataset with WCS; could think about running this with all tests, or always adding a WCS in self.data.coords; however test_subset_legend still fails with this patch, as the version with WCS somehow is not including the subset label:

Basically in the latter case the script is missing these lines:

## Layer 2: mysubset

layer_data = data_collection[0].subsets[0]

# Define a function that will get a fixed resolution buffer of the mask
handle = mpatches.Patch(color='#e31a1c', alpha=0.5)
legend_handles.append(handle)
legend_labels.append(layer_data.label)

so this looks like another actual bug in the exporter (those lines were already missing before this change), as if in

glue/glue/viewers/common/viewer.py

Line 447 in aabb5eb

if layer.visible and layer.enabled:

the subset layer is not visible and enabled when a WCS ist present (indeed layer.enabled = False), but I could not yet figure out why.

Actually comparing to the non-WCS plot it does not seem like the subset is really displayed in the WCS version:

[dhomeier]

OK, looks like the problem was using the wrong Data for the mask, effectively creating an empty (or invalid?) subset (though maybe it should still include the legend in that case?).

[astrofrog]

Looks good but I think we could actually be more general as described below.

[astrofrog]

In principle we should be able to pass any Coordinates subclass to reset_wcs - after all this is what we do in the actual image viewer:

        ref_coords = getattr(self.state.reference_data, 'coords', None)

        if ref_coords is None or isinstance(ref_coords, LegacyCoordinates):
            self.axes.reset_wcs(slices=self.state.wcsaxes_slice,
                                wcs=get_identity_wcs(self.state.reference_data.ndim))
        else:
            self.axes.reset_wcs(slices=self.state.wcsaxes_slice, wcs=ref_coords)

If we don't care about legacy coordinates in the script export, we might be able to get away with just doing:

self.axes.reset_wcs(slices=self.state.wcsaxes_slice, wcs=ref_coords)

if ref_coords is defined? (and not have the case below either)

We could test that this works using e.g. AffineCoordinates.

[dhomeier]

If we don't care about legacy coordinates in the script export, we might be able to get away with just doing:

Basically those are what the else case already covered, but if all the rest can be treated the same way, it becomes much easier of course (I haven't seen an example using coords.wcs or coords.wcs_dict).
No idea if those AffineCoordinates make any sense at all, but they work – apart from the _slice and simple_att tests, that suffer from some fuzziness in the (lower axis) labels. Looks like the exported version does not get the 3D look quite right – can probably live with that?

[astrofrog]

This looks good to me - see comment below about using a fixture to avoid all the boilerplate code added at the start of each test. If you think this can be done, feel free to do it, otherwise you can just merge.

[astrofrog]

I wonder if this could perhaps be implemented as a pytest fixture to avoid the repetition in each test?

[dhomeier]

@astrofrog I implemented this as a helper function now, but could not figure out yet how to use it as a fixture in light of https://docs.pytest.org/en/stable/deprecations.html#calling-fixtures-directly (or if that would actually be helpful).

[astrofrog]

Can you open an issue in the repo to keep track of this?

[astrofrog]

Thanks for adding the helper method! That'll be fine for now, no need to make it a fixture.