Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add area to available statistics #70

Merged
merged 13 commits into from
Sep 23, 2013
Merged

Conversation

astrofrog
Copy link
Contributor

Should be straightforward - need to remember to get the units right if spatial_scale is available.

@ChrisBeaumont
Copy link
Contributor

Good idea. There are a few definitions for area in common use -- one is just the area of the mask (or projected mask in PPV). The other uses sig_maj and sig_min, to give an intensity-weighted spatial dispersion. It's probably best to just pick one for now.

@astrofrog
Copy link
Contributor Author

I was thinking more the actual area of the mask, which is the 'real' area of the structure. The area of the ellipse is easy enough to derive. Though we could have area_exact and area_ellipse or something like this?

@astrofrog
Copy link
Contributor Author

@ChrisBeaumont - what do you think of the attached code? For area_exact for the PPV statistic I was thinking of finding the projected pixel area if the structure is collapsed in the velocity direction. Does this make sense? If so I can add it.

@ChrisBeaumont
Copy link
Contributor

+1. Projecting the PPV cube to compute area_exact sounds good -- Rahul Shetty (and others) have used this definition of area in papers.

@ghost ghost assigned astrofrog Sep 20, 2013
"""
The area of the ellipse representing the first and second moments of the structure.
"""
return np.pi * self.major_sigma * self.minor_sigma
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be 2 pi?

@property
def area_ellipse(self):
"""
The area of the ellipse representing the first and second moments of the structure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the docstring needs to more clearly state whether the ellipse passes through sigma, or HWHM (appears to be the latter)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't think the first moments are relevant here, only the second?

Proposed new docstring:

The area of the ellipse defined by the second moment, where the major and minor axes used are HWHM (half-width at half-max) measures:

.. math::

    \pi HWHM_{maj} HWHM_{min}

though the "math" bit is probably just as easy to say:

area = pi HWHM(maj) HWHM(min)

@ChrisBeaumont
Copy link
Contributor

Looks good, except for the note about the imprecise docstring

@astrofrog
Copy link
Contributor Author

I clarified the docstring. I didn't include the equation because it won't be visible in the docs since this is a property (only the first sentence of text is shown).

@ChrisBeaumont
Copy link
Contributor

Looks good (is the full docstring at least displayed by IPython?)

@astrofrog
Copy link
Contributor Author

@ChrisBeaumont - yes, it should be actually - though in that case I would vote that we can include the equation as 'human-readable' rather than LaTeX since it will never be rendered in LaTeX.

@ChrisBeaumont
Copy link
Contributor

Looks good to me

astrofrog added a commit that referenced this pull request Sep 23, 2013
Add area to available statistics
@astrofrog astrofrog merged commit 52d27c5 into dendrograms:master Sep 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants