Add annulus region #128
Add annulus region #128
Conversation
|
The only fail is with numpy 1.7, that we have already dropped. |
regions/core/tests/test_compound.py
Outdated
| c1 = CirclePixelRegion(pixcoord, 2) | ||
| c2 = CirclePixelRegion(pixcoord, 4) | ||
|
|
||
| union = c1 | c2 |
There was a problem hiding this comment.
Is there anything we can assert here? isinstance(CompoundRegion), say?
| The outer radius of the annulus | ||
| """ | ||
|
|
||
| def __init__(self, center, inner_radius, outer_radius, meta=None, visual=None): |
There was a problem hiding this comment.
A ds9 annulus can have an arbitrary number of rings. I think an in/out annulus, as you've implemented, belongs here, but we also need something to handle the n-annuli case
There was a problem hiding this comment.
I don't really understand the use case for this, what does contains e.g. imply for 5 annuli. Couldn't we return a DS9 n-annulus as a list of n in/out annuli?
There was a problem hiding this comment.
I think there's just some DS9 regions (like text or n-annulus) that are special and don't fit in with regions like circle and polygon that can be used for analysis.
Maybe we have to find a sane way to categorize / handle that?
It's true that contains doesn't make much sense for n-annulus. DS9 has special high-performance code to generate integer-labeled masks for regions in single pixel-pass in C (see e.g. https://github.com/ericmandel/regions) and e.g. the X-ray people use it to do radial profiles in the DS9 GUI.
IMO we shouldn't let DS9 influence the design of this regions package too much.
Not sure what this means concretely for this class and others.
Maybe you can discuss / figure it out in Leiden?
regions/shapes/annulus.py
Outdated
| from ..shapes import CirclePixelRegion, CircleSkyRegion | ||
|
|
||
|
|
||
| class AnnulusPixelRegion(CompoundPixelRegion): |
There was a problem hiding this comment.
This should be called CircleAnnulusPixelRegion because an annulus can have any shape, e.g. we define elliptical and rectangular annulus apertures in photutils.
regions/shapes/annulus.py
Outdated
|
|
||
| class AnnulusPixelRegion(CompoundPixelRegion): | ||
| """ | ||
| An annulus in pixel coordinates. |
There was a problem hiding this comment.
"An annulus" -> "A circular annulus"
regions/shapes/annulus.py
Outdated
| return self.region2.bounding_box() | ||
|
|
||
|
|
||
| class AnnulusSkyRegion(CompoundSkyRegion): |
regions/shapes/annulus.py
Outdated
|
|
||
| class AnnulusSkyRegion(CompoundSkyRegion): | ||
| """ | ||
| An annulus in sky coordinates. |
|
@larrybradley I adressed your comment, ok to merge? |
|
@joleroi Yes, thanks. |
This PR replaces #28
The annulus is now implemented as a compound region of two circles. There is not much functionality, since there is not much functionality in compound regions.