Skip to content

Make PixelAperture.area an abstract method and a lazyproperty#241

Closed
larrybradley wants to merge 4 commits intoastropy:masterfrom
larrybradley:aperture-area
Closed

Make PixelAperture.area an abstract method and a lazyproperty#241
larrybradley wants to merge 4 commits intoastropy:masterfrom
larrybradley:aperture-area

Conversation

@larrybradley
Copy link
Member

Note that placing @lazyproperty on the @abc.abstractmethod doesn't require it to be a lazyproperty in the subclasses. I couldn't find a way to to that, but @abc.abstractmethod will at least require an area method in any pixel Aperture subclasses.

This PR incorporates @cdeil's commit in #223 to make area() and abstract method.

Copy link
Member

Choose a reason for hiding this comment

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

Can you make this an intersphinx link like we do for all other functions / classes from Astropy?

`~astropy.utils.lazyproperty`

I see this has moved form misc.py to decorators.py ... it's now here:
http://astropy.readthedocs.org/en/latest/api/astropy.utils.decorators.lazyproperty.html?highlight=lazyproperty

But this intersphinx-link should work for the old or new version, no?

@cdeil
Copy link
Member

cdeil commented Jan 23, 2015

I looked up what lazyproperty does ... so it caches the value and doesn't re-compute it if the object data members change. So if you do this, shouldn't the aperture data members then be made read-only attributes?
At the moment I can change them and area is always correct:

>>> a = photutils.CircularAperture((1,2), 3)
>>> a.area()
28.274333882308138
>>> a.r = 3.0
>>> a.area()
5541.769440932395

With this change, users would get incorrect results if they access area, then change a data member and then access it again, no?

@larrybradley Maybe it's simpler to re-compute area instead of making all aperture data members read-only? Is this really a performance bottleneck?

@larrybradley
Copy link
Member Author

@cdeil I didn't realize that the aperture shapes could be changed that way. Yes, in that case we should keep area() as a method and not a lazyproperty. I'll close this PR, so go ahead and merge #223.

@larrybradley larrybradley deleted the aperture-area branch January 28, 2015 15:11
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.

2 participants