dendrograms / astrodendro

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

WCS loading/saving enabled for dendrograms #137

Closed tomr-stargazer closed 9 years ago

tomr-stargazer commented 9 years ago

Hi all (especially @ChrisBeaumont),

I decided I needed to see if dendrogram saving/loading with WCS objects (enabled at compute-time since #126) could be easily implemented. This is my quick first attempt at getting it to work. From my point of view it doesn't break too bad, but I haven't tested many cases, and admittedly I didn't try very hard to learn about the internals of the hdf5 module.

Especially let me know if I'm addressing this at the wrong level of abstraction.

tomr-stargazer commented 9 years ago

After playing around just a bit more, this code seems to be doing what I want - so I'll be using this in production. If desired I could try to add tests for wcs and non-wcs'd reloading.

tomr-stargazer commented 9 years ago

Also, there are weird Travis issues coming up: https://travis-ci.org/dendrograms/astrodendro/jobs/54980582

    ----------------------------------------
    Command "/home/travis/miniconda/envs/test/bin/python -c "import setuptools, tokenize; __file__='/home/travis/build/dendrograms/astrodendro/src/astropy/setup.py'; exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" develop --no-deps" failed with error code 1 in /home/travis/build/dendrograms/astrodendro/src/astropy
The command "if [[ $ASTRO_VER == dev ]]; then pip install -e git+git://github.com/astropy/astropy.git#egg=astropy; else $CONDA_INSTALL astropy=$ASTRO_VER numpy=$NP_VER; fi" failed and exited with 1 during .
Your build has been stopped.

which seem version-dependent.

ChrisBeaumont commented 9 years ago

This looks good on the hdf5 side! It would be great to add a test. It also looks like the fits exporter/importer needs this patch.

ChrisBeaumont commented 9 years ago

@astrofrog what do you make about the jinja2 import error when pip installing astropy? https://travis-ci.org/dendrograms/astrodendro/jobs/54980585

We can probably fix this in astrodendro by installing jinja2 explicitly

astrofrog commented 9 years ago

Ah yes, we need to now include jinja2 where we include Cython.

@tomr-stargazer - can you do this in this PR?

astrofrog commented 9 years ago

Basically we just need to add jinja2 to CONDA_BASE

astrofrog commented 9 years ago

Just for some background, this is due to the fact that astropy-dev now requires Cython and jinja2 to build.

tomr-stargazer commented 9 years ago

Hi @astrofrog - I've just added jinja2 in the requisite place. (I haven't added @ChrisBeaumont's tests yet or any other changes so don't merge quite yet)

tomr-stargazer commented 9 years ago

Hi @ChrisBeaumont @astrofrog et al: I've just added the FITS functionality and tests for both hdf5 and fits saving/loading. I think the tests should pass. Do let me know if this is a clumsy implementation of saving a WCS header for the fits patch, as I'm not a fits/HDU pro.

ChrisBeaumont commented 9 years ago

Looks good! There's a binary file in this PR astrodendro/tests/astrodendro-test, I assume that isn't intended?

tomr-stargazer commented 9 years ago

@ChrisBeaumont Thanks for the comments, I've addressed each issue!

tomr-stargazer commented 9 years ago

@ChrisBeaumont @astrofrog Tests pass and I've addressed all issues, so I think this should be ready to merge, unless there are other concerns. (also, this will close #129)

ChrisBeaumont commented 9 years ago

Looks good, thanks!