search

dendrograms / astrodendro

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

Initial Code for dendro-core #1

Closed bradenmacdonald closed 11 years ago

bradenmacdonald commented 11 years ago

This pull request provides a basic implementation of the Python dendrogram code, stripped of extraneous features, and more or less refactored along the lines of the "API Proposal" wiki discussion. Please read through the code and check it out! The best place to see examples of the code in use would be the unit tests, in the "tests" directory.

astrofrog commented 11 years ago

Looks great! I'm going to leave some inline comments regarding a few issues I came across. To add commits to this PR, just add commits to this branch and they will automatically appear here. Try and avoid rebasing for now though.

astrofrog commented 11 years ago

The PyFITS and h5py imports should be moved inside the functions that use them, so that the file can still be imported without the dependency. In addition, we should switch from pyfits to:

from astropy.io import fits

Using Astropy as a dependency would be nice for other reasons too - for example you basically can also use the ProgressBar class from there, which would avoid code duplication:

http://docs.astropy.org/en/latest/_generated/astropy.utils.console.ProgressBar.html#astropy.utils.console.ProgressBar

In [8]: p = ProgressBar(100)
|>-----------------------------------------|   0/100  (  0.00%)
In [9]: p.update(30)
|============>-----------------------------|  30/100  ( 30.00%) ETA    04s
In [10]: p.update(60)
|=========================>----------------|  60/100  ( 60.00%) ETA    03s
In [11]: p.update(100)
|==========================================| 100/100  (100.00%)        08s

the ETA feature is nice. What do you think?

astrofrog commented 11 years ago

By the way, there are other reasons why it would be nice to have astropy as a dependency - for example, we can make use of some of the testing and documentation framework in there, etc. (I can submit a pull request making use of these things at a later time).

astrofrog commented 11 years ago

Apart from the above comments, this looks great to me! Once @ChrisBeaumont has had a chance to review this and his comments have been addressed, I suggest we merge. There's some improvements I'd like to make to the testing (including adding more tests) but this doesn't have to all be done in this initial PR.

bradenmacdonald commented 11 years ago

Using astropy in the future definitely makes sense, and having an ETA for the progress bars would be cool :sunglasses:

Regarding "The PyFITS and h5py imports should be moved inside the functions that use them": Actually, I coded this so all the "Import/Export plugins" in the astrodendro.io.* package are lazy-loaded only when the user requests that particular file type. For example, when you say my_dendrogram.save_to("file.hdf5"), the save_to method sees the hdf5 file extension and then loads the astrodendro.io.hdf5 module at that time. Thus, someone who doesn't have h5py installed can use astrodendro without any errors, as long as they don't try to load/save an HDF5 file.

astrofrog commented 11 years ago

Regarding Astropy - maybe we can wait until after this PR and for the 0.2 release of Astropy (in <1 month) to do that and I can submit a PR then to update that in dendro-core. I realize now that the I/O modules can be lazy loaded - thanks for the explanation!

ChrisBeaumont commented 11 years ago

This looks nice to me (disclaimer: I haven't used it beyond installing and running the tests). I'll make some minor comments in-line, but otherwise I'm fine with merging and improving, as Tom suggests.

bradenmacdonald commented 11 years ago

Thanks for all the feedback, Tom and Chris. I think this may now be ready to merge?

astrofrog commented 11 years ago

It looks great - thanks for all your work on this! Of course, we may run into small issues as we use it, but we should just go ahead and merge it and we can make any changes in subsequent PRs, which will be more manageable. I also would like to add some additional tests, and switch to astropy for FITS I/O.

As a rule, I suggest that we always make any changes via pull requests, unless they are clearly trivial (typos for example). What do you think?

@ChrisBeaumont - good to merge?

ChrisBeaumont commented 11 years ago

This looks good to me! Thanks Braden

chris

On Thu, Oct 18, 2012 at 5:08 PM, Thomas Robitaille <notifications@github.com

wrote:

It looks great - thanks for all your work on this! Of course, we may run into small issues as we use it, but we should just go ahead and merge it and we can make any changes in subsequent PRs, which will be more manageable. I also would like to add some additional tests, and switch to astropy for FITS I/O.

As a rule, I suggest that we always make any changes via pull requests, unless they are clearly trivial (typos for example). What do you think?

@ChrisBeaumont https://github.com/ChrisBeaumont - good to merge?

— Reply to this email directly or view it on GitHubhttps://github.com/dendrograms/dendro-core/pull/1#issuecomment-9581362.

Chris Beaumont Graduate Student Institute for Astronomy University of Hawaii at Manoa 2680 Woodlawn Drive Honolulu, HI 96822 www.ifa.hawaii.edu/~beaumont