search

glue-viz / glue

Linked Data Visualizations Across Multiple Files
http://glueviz.org
Other
742 stars 153 forks source link

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

Closed dhomeier closed 2 years ago

dhomeier commented 2 years ago

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: expected glue_plot

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 https://github.com/glue-viz/glue/blob/aabb5ebb1b5227f973a99f9aa905d6bfe6988b0b/glue/viewers/common/viewer.py#L447

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: glue_plot

dhomeier commented 2 years ago

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 commented 2 years ago

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