dendrograms / astrodendro

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

compute_flux() now accepts Kelvin brightness temperature units #112

Closed tomr-stargazer closed 10 years ago

tomr-stargazer commented 10 years ago

I have added support for Kelvin main beam brightness temperature units in astrodendro.flux.compute_flux(). While previous discussions have noted (e.g., #54, #57) that it may be better to provide a function to convert Kelvin datacubes to less ambiguous units before performing analysis computations, my use-case is better served by doing the computations directly on Kelvin data (rather than transforming my datacubes to Jy/beam before running the catalog analysis) - for example, I'd like my dendrograms to remain in Kelvins rather than be converted to Jy/beam. As @astrofrog noted in an email from 1 Feb 2014, astropy now has a "brightness temperature" equivalency between K and Jy/beam, so this pull request uses this equivalency.

I have included a "warning"-style print statement in the code, so that "Warning: 'Kelvin' units interpreted as main beam brightness temperature." is printed each time this computation runs. I have also updated the docs to reflect the ability to use Kelvin units. Finally, I included a new test case in test_compute_flux that uses K units with the appropriate metadata.

This code should pass Travis tests, but I'll address any failures if they come up.

Note that this addition sidesteps the still-relevant issue raised in #107, which is concerned with renaming "compute_flux" to "compute_flux_density" and ensuring that spectral line datacubes are treated properly when summed over a frequency or velocity axis.

tomr-stargazer commented 10 years ago

I just discovered a mathematical error (I shouldn't have excluded the beams_per_pixel calculation) which I'll upload the fix for in the morning.

Also, I forgot that Travis also tests against python3, so I'll fix the non-function print statement. If you know of a better way to pass that kelvin warning besides a print, do let me know (:

ChrisBeaumont commented 10 years ago

As @astrofrog noted in an email from 1 Feb 2014, astropy now has a "brightness temperature" equivalency between K and Jy/beam, so this pull request uses this equivalency.

I'm not sure when this feature was introduced -- is it a 0.4 feature? If it's not a big pain, I'd prefer that we add a workaround to support 0.3. If that isn't feasible, can we add an astropy version check, to alert the user that astropy 0.4 is required if (and only if) they want to use K flux units?

keflavich commented 10 years ago

It's an 0.3 feature: http://docs.astropy.org/en/stable/changelog.html?highlight=brightness%20temperature#id4 (link was edited to highlight the right thing)

tomr-stargazer commented 10 years ago

Thanks much for the comments, guys! I think I've addressed all of them, so if this commit passes Travis tests I believe we should be ready to merge (although I could do some commit squashing if desired).

ChrisBeaumont commented 10 years ago

This looks good to me. It'd be great if @astrofrog chimes in, since he is the original advocate for delaying treatment of K.

astrofrog commented 10 years ago

This looks great! Since this comes with a warning, +1 from me!