search

dendrograms / astrodendro

Generate a dendrogram from a dataset
https://dendrograms.readthedocs.io/
Other
37 stars 36 forks source link

WCS at dendrogram compute time, for WCSAxes-viewer integration #126

Closed tomr-stargazer closed 9 years ago

tomr-stargazer commented 9 years ago

This is a few simple changes that allow you to pass a WCS instance to astrodendro.Dendrogram.compute and then automatically have the WCS coordinates properly applied when bringing up Dendrogram.viewer(). I've tested it on both 2d and 3d data and it seems to work. This addresses Issue #96.

Currently the code trusts the user not to provide an incorrect WCS (i.e., there are no checks for data size or dimensionality).

Thoughts:

tomr-stargazer commented 9 years ago

Looks like this for 2d data:

In [16]: example_2d_data = getdata('astrodendro/docs/PerA_Extn2MASS_F_Gal.fits')
In [19]: example_2d_header = getheader('astrodendro/docs/PerA_Extn2MASS_F_Gal.fits')
In [20]: example_2d_wcs = wcs.WCS(example_2d_header)
In [21]: example_2d_dendro = astrodendro.Dendrogram.compute(example_2d_data, wcs=example_2d_wcs)
In [23]: example_2d_dendro.viewer()
Out[23]: <astrodendro.viewer.BasicDendrogramViewer at 0x10f57b690>

image

tomr-stargazer commented 9 years ago

Similarly, for 3d data: image

tomr-stargazer commented 9 years ago

My latest commit addresses @ChrisBeaumont's suggestion to make wcsaxes an optional import, but before merging, see my comment in response to that "outdated diff" above.

ChrisBeaumont commented 9 years ago

This looks fine to me -- if you want to fallback to non-wcsaaxes instead of raising an error that makes sense (and is probably best). Other than that I'm fine with merging

tomr-stargazer commented 9 years ago

@ChrisBeaumont I've changed it to just a "warn, and then fallback to default behavior" (with state mediated by a goofy __wcsaxes_imported variable -- this seemed like the cleanest way to get this logic to work, though perhaps a little unconventional in style).

I don't really know how to test the "wcsaxes is not installed" behavior at the moment, though.

tomr-stargazer commented 9 years ago

Update: I "tested" (cough) the fallback behavior by forcing an ImportError, and got the result I wanted:

In [3]: dv = d.viewer()
/Users/tsrice/Documents/Code/astrodendro/astrodendro/viewer.py:95: UserWarning: `WCSAxes` package required for wcs coordinate display.
  warnings.warn("`WCSAxes` package required for wcs coordinate display.")
# the viewer loaded and displayed with pixel-coordinate axes, as expected

so this probably works.

tomr-stargazer commented 9 years ago

Also, Travis seems to be having a bad day with pip or astropy today as all of the test builds are consistently erroring on the following error: The command "pip -q install astropy" failed and exited with 1 during .

... but all tests are passing on my machine (python 2.7.8, numpy 1.8.1), for what that's worth.

astrofrog commented 9 years ago

@tomr-stargazer - Huh, @embray released a new tarfile today, I wonder if it's related. Can you remove the -q flag from the pip install temporarily?

tomr-stargazer commented 9 years ago

@astrofrog it doesn't seem to have helped The command "pip install astropy" failed and exited with 1 during .

astrofrog commented 9 years ago

@tomr-stargazer - that did help, it is an issue with the latest tar file release: https://github.com/astropy/astropy/issues/2848

tomr-stargazer commented 9 years ago

The Travis errors are mostly gone (except for that Python 2.6 build, not sure if that's serious), and I think this PR was ready to merge unless anyone else had comments on the code?

ChrisBeaumont commented 9 years ago

The 2.6 error looks like another potential astropy issue. I restarted the build, but will merge this if that test fails again.