search

dendrograms / astrodendro

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

Definition of bmaj/bmin in analysis should be changed #40

Closed astrofrog closed 11 years ago

astrofrog commented 11 years ago

In the analysis module, bmaj/bmin are used as sigma, but in the FITS standard they are the FWHM, so this will lead to confusion. I think we should change it so they are defined as FWHM too - does this make sense?

ChrisBeaumont commented 11 years ago

sounds good

astrofrog commented 11 years ago

Ok, I will do this once #39 is merged.

astrofrog commented 11 years ago

@ChrisBeaumont - I would like to propose that we actually remove BMIN/BMAJ from the analysis code - the reason why is that beam smearing only happens for certain kinds of images but doesn't make sense for a lot of others, and we don't want users to be warned about them in all cases. Even if we disabled warnings for these meta-data if they are missing, the deconvolution is only approximate if the beam is at a different position angle to that of the structures (we should really ask users for BPA too, the position angle of the beam). I would actually prefer if we add a section in the docs showing how easy it is to compute derived quantities such as this, but I think this is too specialized and inexact to be in the core analysis code (I'd be worried about users trusting the value blindly, whereas if they compute it themselves, they will understand better what approximations they are making). What do you think?

ChrisBeaumont commented 11 years ago

@low-sky might have an opinion here. I took the deconvolution equation from one of his scripts. I agree that this makes several assumptions (structures are gaussian, with PAs aligned with beam PA). And you're right that it isn't applicable in all situations.

I'm fine with removing this (or making it some kind of optional keyword option)

low-sky commented 11 years ago

I actually thought that @astrofrog had a good point here. Doing the deconvolution post-hoc would make a lot of sense since it is more of a special case than you want for this now highly general code to deal with.

astrofrog commented 11 years ago

@low-sky - thanks for the feedback. I'll submit a pull request!

astrofrog commented 11 years ago

If the tests pass, I'll merge this if there are no objections :)

ChrisBeaumont commented 11 years ago

:+1: