Closed ChrisBeaumont closed 11 years ago
Thanks for implementing this! The header is not ideal because keys are 80 characters long max and will have to resort to continuation markers, etc. - gonna be messy. A cleaner method would be to use a table with a single cell and just put it in there since there are no limits in that case as far as I can tell. Not ideal in terms of FITS structure, but the best I can think of.
Ok, I'm saving the newick string as a byte array in its own extension. In principle I could save it as a string, but the IDL fits routines don't properly read extensions consisting of string data (even though they seem to get written properly). I figure it's worth sidestepping this issue
I ran my IDL analysis code on 2D and 3D datasets, and was able to reproduce the same numbers with the python version (the only quantity that wasn't tested was sky_pa, which I didn't implement in IDL). The only change to the python code (commit e363b1c) is a better default value for the vaxis
metadata item.
I haven't made this a formal test in this repo, but I could if you think it should be added
Nice! I haven't had a chance to review in detail, but this looks great! Thanks for doing the comparison to IDL! I don't think that this needs to be added as a formal test, since we do already have tests of the dendrogram computation, and if the algorithm changes, I imagine the tests will fail.
Sounds reasonable to me. This is then ready to review when you have time
I'm reviewing this now - just a quick note, it might be worth updating the docs too to mention FITS export/import is possible:
http://www.dendrograms.org/en/latest/using.html#saving-the-dendrogram
I just reviewed this, and it looks good to me! (apart from the comment about adding the docs)
Now that we have this, I'd also like to include a few more tests of the dendrogram computation, i.e. we include real data (though not very large files) in the repository, and we store the expected dendrogram in a FITS file - then the test checks that the computed dendrogram does not change. There is a similar kind of test at the moment though it does not check that the full dendrogram is identical structure for structure. This can be left to a separate PR though.
Im without internet access today and tomorrow, but I will add some docs and a test like you mention. On Jul 4, 2013 10:23 AM, "Thomas Robitaille" notifications@github.com wrote:
I just reviewed this, and it looks good to me! (apart from the comment about adding the docs)
Now that we have this, I'd also like to include a few more tests of the dendrogram computation, i.e. we include real data (though not very large files) in the repository, and we store the expected dendrogram in a FITS file - then the test checks that the computed dendrogram does not change. There is a similar kind of test at the moment though it does not check that the full dendrogram is identical structure for structure. This can be left to a separate PR though.
— Reply to this email directly or view it on GitHubhttps://github.com/dendrograms/dendro-core/pull/50#issuecomment-20482546 .
@astrofrog I updated the docs. Feel free to merge
Great, thanks!
I have no idea how to read hdf5 files in IDL so, to compare the catalog code from IDL, I need fits import/exportability
One question for @astrofrog: right now I'm putting the newick string in the header. These strings could potentially be large (several KB). Is that a problem? Is there a more natural place to put a single large string in a fits file?