Skip to content

Change copy and add rotate methods#281

Merged
cdeil merged 5 commits intoastropy:masterfrom
cdeil:rotate
Jun 16, 2019
Merged

Change copy and add rotate methods#281
cdeil merged 5 commits intoastropy:masterfrom
cdeil:rotate

Conversation

@cdeil
Copy link
Member

@cdeil cdeil commented Jun 13, 2019

This PR is a follow-up to #269 - I did implement the suggestion from #269 (comment) and put copy in the base class. I'm not sure if that's good - comments welcome!

This PR also adds PixCoord and pixel region rotate methods, as I mentioned in #265 .

I'll continue a bit and add copy and rotate for a few more regions here.

@astrofrog @keflavich @registerrier or anyone - please review.

@cdeil cdeil added this to the 0.4 milestone Jun 13, 2019
@cdeil cdeil requested review from astrofrog and keflavich June 13, 2019 21:18
@cdeil cdeil self-assigned this Jun 13, 2019
@larrybradley
Copy link
Member

Each region already defines a _repr_params attribute for the shape parameters. With the exception of polygons, your _fields attribute simply adds center, meta, visual. Can you simply use _repr_params and add [center, meta, visual] in the copy method?

Polygons would require a workaround, but I think the polygon API should be changed (but not in this release!) so that its "center" can be moved around by changing one parameter instead of all the vertices. IIRC we discussed that at one time.

@cdeil
Copy link
Member Author

cdeil commented Jun 13, 2019

@larrybradley - Agreed that both _fields and _repr_params is bad. But I would prefer a tuple of fields, where for each field one says if it should be repr=True or repr=False as in https://docs.python.org/3/library/dataclasses.html . Not sure if we can achieve that for now.

Is it really useful to exclude center from _repr_params? Isn't this often the most important info about a region?

Another cosmetic change I'd suggest is to move these tuples to class-level, out of __init__. We don't need to access them from the class for now, but again I'd suggest to follow the design in https://docs.python.org/3/library/dataclasses.html a bit.

@cdeil
Copy link
Member Author

cdeil commented Jun 13, 2019

@larrybradley - I followed your suggestion in 19edcfc - for now we just have _repr_params as was already the case. I've written the copy in the base class so that it also works for polygons.

Polygons would require a workaround, but I think the polygon API should be changed (but not in this release!) so that its "center" can be moved around by changing one parameter instead of all the vertices. IIRC we discussed that at one time.

You proposed it in #57. It wasn't clear to me why it's useful. Is it a performance issue, to have an efficient "shift" of a polygon to a new position, in case there are many vertices and you don't want to make another array?
I'm not really opposed to add a center or position attribute for polygons, but given that it does add some extra complexity (and probably isn't available for sky polygons?) - there should be a clear motivation for it.

@cdeil
Copy link
Member Author

cdeil commented Jun 13, 2019

Added rotate for circle, ellipse, rectangle and polygon. Seems to work fine, based on the test added.

Ready for final review.

@cdeil cdeil requested a review from larrybradley June 13, 2019 22:33
@cdeil cdeil merged commit d324f2e into astropy:master Jun 16, 2019
This was referenced Jun 16, 2019
@larrybradley
Copy link
Member

@cdeil I think the _repr_params didn't include the center property because it's assumed that most regions have a center, so the center was added once to _repr_params in the base class, i.e. _repr_params only included the parameters defining the shape, which differ from region to region.

I think it's a good idea to move the params tuple to the class level. I'm not familiar with the @dataclass decorator, but the examples seem to all use type annotations, which I'm not a big fan of.

@larrybradley
Copy link
Member

@cdeil For the polygon region, I'm thinking of the use case of aperture photometry. In that case, one has an (x, y) position/centroid of a source on which they want to perform photometry within a particular aperture. Therefore, it would be nice for polygon regions to have the same API as the other regions that use center, e.g.:

xypos = PixCoord(10, 20)
reg1 = CirclePixelRegion(xypos, radius=3)
reg2 = PolygonPixelRegion(xypos, vertices=vertices)
aperture_photometry(data, (reg1, reg2))

where the polygon vertices are defined relative to (0, 0). In other words vertices defines the shape of the polygon and position defines its position. That API mirrors all the other regions that separate the center parameter from the shape parameters, except we can't call it center for polygons because depending on how vertices are defined, it may not be the true center (although for aperture photometry it should be the true center).

xypos2 = PixCoord(30, 40)
 # no need to recompute polygon vertices for new position
reg3 = PolygonPixelRegion(xypos2, vertices=vertices)

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