Skip to content

Make PixelAperture.area an abstract method#223

Merged
cdeil merged 1 commit intoastropy:masterfrom
cdeil:PixelAperture.area
Jan 23, 2015
Merged

Make PixelAperture.area an abstract method#223
cdeil merged 1 commit intoastropy:masterfrom
cdeil:PixelAperture.area

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 21, 2014

@bsipocz @astrofrog Does it make sense to make PixelAperture.area an abstract method?

All tests pass before or after this change ... I made it because my editor was complaining here about the missing self.

@cdeil cdeil added this to the 0.2 milestone Dec 21, 2014
@cdeil cdeil self-assigned this Dec 21, 2014
@cdeil
Copy link
Member Author

cdeil commented Dec 21, 2014

This change is not consistent with area being described as an optional method here:
http://photutils.readthedocs.org/en/latest/photutils/aperture.html#defining-your-own-custom-apertures

It says there:

area(): If convenient to calculate, this returns the area of the aperture. This speeds computation in certain situations (such as a scalar error).

But I can't find a point of use of area() ... is this implemented?

@bsipocz
Copy link
Member

bsipocz commented Dec 28, 2014

@cdeil - I think we didn't make it an abstract method as it may not be straightforward to define in future or user defined aperture classes.

However the method may well became obsolete, as it was only used for some speeding up when calculating the errors.

@larrybradley
Copy link
Member

I think the area() method is useful. It should be easy to define for any realistic (e.g. simple geometric shape) apertures. So, I'm 👍 for making it an abstract method (with the corresponding doc changes). I also think it should be made a "lazyproperty":
http://astropy.readthedocs.org/en/stable/api/astropy.utils.misc.lazyproperty.html?highlight=lazy#astropy.utils.misc.lazyproperty

One concrete example where area() is useful is local background subtraction, where one needs the mean value within the aperture. area() wasn't used in the example here: http://photutils.readthedocs.org/en/latest/photutils/aperture.html#local-background-subtraction, but I will update that.

@cdeil
Copy link
Member Author

cdeil commented Jan 21, 2015

@larrybradley So what do I have to do ... just add @lazyproperty in front of the base class method?

Feel free to do this yourself somewhere else if it's easier that explaining to me what to do.

@larrybradley
Copy link
Member

@cdeil I've never used it with a base class. I'd try that first, but you may need to add @lazyproperty in front of every area() method too.

@cdeil
Copy link
Member Author

cdeil commented Jan 21, 2015

And I guess tests should be added ... I'll try this on Saturday, but if someone gets to it before I'd be happy.

cdeil added a commit that referenced this pull request Jan 23, 2015
Make PixelAperture.area an abstract method
@cdeil cdeil merged commit 5698c3b into astropy:master Jan 23, 2015
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.

3 participants