Improve shape classes and MPL plotting #85
Conversation
We usually have two classes per file in `regions.shapes` at the moment. One for the pixel, one for the sky region. IMO it's better to have tests clearly grouped per class, as much as possible, I find that easier to maintain.
| xy = self.center.x, self.center.y | ||
| width = 2 * self.major | ||
| height = 2 * self.minor | ||
| # TODO: think about how to map major / minor / angle to MPL parameters. |
There was a problem hiding this comment.
@larrybradley @astrofrog - OK if I change the argument order for ellipse to major / minor?
This seems more natural to me and is more in line with http://photutils.readthedocs.io/en/latest/api/photutils.EllipticalAperture.html .
What about the angle passed to MPL.
Does anyone know if it's OK as defined here?
I was looking at this for guidance:
http://photutils.readthedocs.io/en/latest/_modules/photutils/aperture/ellipse.html#EllipticalAperture.plot
There was a problem hiding this comment.
Same for rectangle ... I'll go ahead and follow the lead of MPL and photutils and change to width/height argument order:
http://photutils.readthedocs.io/en/latest/api/photutils.RectangularAperture.html
Let me know if you disagree.
There was a problem hiding this comment.
I'm not sure where you changed the major/minor order, but what you have here is correct for the ellipse. Photutils defines the theta as the counterclockwise rotation of the semimajor axis from the x axis. This is the same for the EllipsePixelRegion.
There was a problem hiding this comment.
I don't see any changes to rectangle. photutils.RectangularAperture is correct as defined, where the width side is along the x axis for theta=0 and theta increases counterclockwise.
Note that the all of these plots require the "normal" display convention where the (0, 0) pixel is at the lower left (i.e. origin='lower'). Otherwise the rotation angle will be in the opposite direction.
There was a problem hiding this comment.
@larrybradley - Thank you. I've fixed added two more commits here that fix up the rectangle as_patch and added a plot to the docstring for rectangle and ellipse. Docstring updates are deferred to #86
| assert_allclose(patch.xy, (3, 4)) | ||
| assert_allclose(patch.get_width(), 4) | ||
| assert_allclose(patch.get_height(), 3) | ||
| assert_allclose(patch._angle, 5) |
There was a problem hiding this comment.
@astrofrog @larrybradley - I couldn't find a public attribute or method to get at the angle in a matplotlib.patches.Rectangle. Does it exist!?
There was a problem hiding this comment.
@cdeil I looked in the matplotlib source code and there's no public attribute or method (that's odd), only the _angle attribute.
|
@astrofrog - Is this a bug in |
|
@astrofrog @larrybradley - This is ready for final review. |
|
The |
|
Concerning |
|
I lost track of all the plotting discussion, however the current state of the compound regions example plot does not look satisfactory at all, does it? http://astropy-regions.readthedocs.io/en/latest/getting_started.html#compounds As far as I understand, @cdeil does not want to merge #77.
So how are we going to improve the plotting issue? |
|
@joleroi and I discussed this over lunch. The plot isn't incorrect, it just plots pixel regions. I agree it's confusing for illustrating compounds, the points and regions should be consistently in pixels or in sky, or there could even be one example for each. Also, there should be an illustration of the differences between pixel and sky regions further up in the docs. I don't have time, and I think we should start by implementing our polygon classes, and only then come back to docs / testing for sky regions. But like I said in #63, if someone has time to make a pull request to improve |
This PR:
as_patchandplotas discussed in SkyCircleRegion matplotlib plot issue #76 and Fix plotting of SkyCircleRegion #77EllipsePixelRegion.as_patch(and add example to docstring)RectanglePixelRegion.as_patch(and add example to docstring)as_patchmethods