Added initial implementation of Polygon to_mask and as_patch #87

astrofrog · 2016-11-30T14:14:06Z

This implements to_mask for Polygon in line with other shapes.

It does not use code from ginga for example - I did look at that code, but the issue is we need to be able to efficiently compute the sub-pixel overlap without making large arrays of pixels (the ginga code operates with vectorized operations on Numpy arrays).

However, the code I'm adding here for the point/polygon stuff is pretty lightweight. Example usage:

import numpy as np
import matplotlib.pyplot as plt
from regions import PolygonPixelRegion, PixCoord

vx = np.array([-3, 5, 2, 0.5, -1]).astype(float)
vy = np.array([-2., -3., 4., 2., 3.]).astype(float)

vertices = PixCoord(vx, vy)

region = PolygonPixelRegion(vertices)

mask = region.to_mask(mode='subpixels', subpixels=10)

fig = plt.figure()
ax = fig.add_subplot(1, 1, 1)
ax.imshow(mask, cmap=plt.cm.viridis, origin='lower',
          interpolation='nearest',
          extent=mask.bbox.extent)
ax.add_patch(region.as_patch(edgecolor='white', facecolor='none'))
fig.savefig('polygon.png')

This needs tests of course, but waiting first to see if people are open to this.

cdeil · 2016-11-30T14:38:04Z

@astrofrog - Awesome!

I have to run now, but I'll try to review tonight.

What's the origin of the code in regions/_geometry/pnpoly.pyx? Which project? Do you have a URL where you got it?

# The code in this file was adapted from code written by Greg von Winckel and
# which was released under the following license:
#
# ----------------------------------------------------------------------------
#
# The MIT License (MIT)
#
# Copyright (c) 2014 Greg von Winckel

I think we should also acknowledge them in the docs or changelog.

The ideal situation would be if they agree to contribute the code under our BSD-3 license via a comment here or by email, just so that we don't complicate the license situation of Astropy.
Alternatively, having some MIT licensed code in Astropy is fine of course as well.

astrofrog · 2016-11-30T14:45:47Z

The ideal situation would be if they agree to contribute the code under our BSD-3 license via a comment here or by email, just so that we don't complicate the license situation of Astropy.
Alternatively, having some MIT licensed code in Astropy is fine of course as well.

Just to be clear, when we take MIT code and we put it in Astropy, we are re-licensing it as BSD but just acknowledging the original license. So the code is not currently MIT, we are releasing it in a BSD package, but we're just including the original license for reference.

astrofrog · 2016-11-30T14:45:59Z

I added the URLs where the original code was adapted from

astrofrog · 2016-11-30T14:47:07Z

Another note: I think there are some inefficiencies in the Cython code at the moment that could be sped up, but I'm not too concerned about performance right now. I think maybe once we have a lot of the functionality in place we could do a performance sprint where we try and speed everything up where needed?

cdeil · 2016-11-30T21:07:56Z

Just to be clear, when we take MIT code and we put it in Astropy, we are re-licensing it as BSD but just acknowledging the original license. So the code is not currently MIT, we are releasing it in a BSD package, but we're just including the original license for reference.

Thanks. The origin / license is now clearer in regions/_geometry/pnpoly.pyx.

Still, it doesn't say what you write in that comment. If we're re-licensing as BSD, then I'd suggest the first line in the file to be our usual

# Licensed under a 3-clause BSD style license - see LICENSE.rst

and then after that to explain the origin and keep the MIT license.

In the end it's just ~ 15 lines of code. We could just do a BSD licensed clean-room implementation before putting it in Astropy core, i.e. one person writes down or links to a description of the algorithm, and a second writes the code from that only. I've always wanted to do that. But not now, I'm OK to merge this and will shut about license now and have a look at the code. :-)

cdeil · 2016-11-30T21:16:13Z

regions/shapes/polygon.py

@@ -35,8 +40,10 @@ def area(self):
        raise NotImplementedError

    def contains(self, pixcoord):
-        # TODO: needs to be implemented
-        raise NotImplementedError
+        return points_in_polygon(pixcoord.x.astype(float),


There's this error:
https://travis-ci.org/astropy/regions/jobs/180103544#L819

E AttributeError: 'int' object has no attribute 'astype'

Besides fixing the error, may I suggest you change the code to do the type conversions on separate lines before the points_in_polygon call and put them in temp variables?
The efficiency is the same, and the error message for invalid data like here will improve.
At the moment from the traceback it's not clear which variable / line is the problematic one.

Should be fixed

cdeil · 2016-11-30T21:20:12Z

In your first comment on this PR, you have a plot with a polygon and a mask. Very nice!
Please add it to the docs, either the polygon class docstring, or the high-level getting started docs section.

astrofrog · 2016-11-30T21:21:42Z

@cdeil - the 'getting started' documentation is growing quite long - what do you think about splitting it up into multiple pages?

cdeil · 2016-11-30T21:21:58Z

regions/_geometry/pnpoly.pxd

@@ -0,0 +1,5 @@
+cimport numpy as np


Add standard license line at the top, even if it's a very small file.

Can you add a docstring with a sentence saying why we need a pxd file at all?
(my Cython is always rusty because I use it so rarely. It's the same for most Python programmers.)

My knowledge of Cython is 'because Cython complains if it's not there' ;)

Done (it's because it's needed for cimport to work)

cdeil

I've left two more small inline comments.

I didn't review the algorithm / math.

@astrofrog - Should we merge this when travis-ci passes, or do you want more review here?
Maybe we leave it open for a day or two, and if people want to contribute tests or docs, they can make pull requests against this branch?
Or we merge in master now, to increase the chance this gets some usage before the regions 0.2 release?

cdeil · 2016-11-30T21:25:38Z

regions/_geometry/pnpoly.pyx

+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+# SOFTWARE.
+
+cimport cython


I think this line can be removed!?

cimport cython

cdeil · 2016-11-30T21:26:23Z

regions/_geometry/pnpoly.pyx

+
+    for i in range(n):
+        j = (i+n-1) % n
+        if(((vy[i] > y) != (vy[j] > y)) and \


This if statement spanning over 4 lines is very hard to read.
Can you compute the parts on separate lines and store them in temp variables, and then have a one-line conditional?

The 'and' short-circuits the evaluation of the second part of the if statement, so I don't want to pre-compute the second part - but I'll clean it up.

PyCharm suggests to remove the backslashes, and I agree, I think generally just parens to group multi-line expressions are the preferred style over backslashes. Not sure if that's PEP8 or not.

astrofrog · 2016-11-30T21:31:25Z

@cdeil - I'll add an arraydiff test now so there is at least some form of testing. But we can merge before the docs. I think for 0.2 it'd be nice to split up the docs into more pages - at the moment 'getting started' is a bit monolithic - would you have time to work on that?

cdeil · 2016-11-30T21:32:00Z

the 'getting started' documentation is growing quite long - what do you think about splitting it up into multiple pages?

I was thinking about this today, too.
Yes, splitting it onto multiple pages is fine with me.

The other idea I had was that we create an IPython notebook with some nice demos to get people's attention. We could make it a binder, so that they can try it out online.
The only problem with that is that we grow the repo size, probably by a few MB with images, and I don't think this is something we want to end up in the Astropy core repo at the end.

astrofrog · 2016-11-30T21:32:54Z

The only problem with that is that we grow the repo size, probably by a few MB with images, and I don't think this is something we want to end up in the Astropy core repo at the end.

What about putting that in the astropy-tutorials repo? It's already on binder that way.

cdeil · 2016-11-30T21:33:08Z

would you have time to work on that?

Very little, but yes, I'll try to put some more time into docs before making the release.

cdeil · 2016-11-30T21:42:11Z

What about putting that in the astropy-tutorials repo?

Yes, probably that would be the best solution.
Made an issue as a reminder and for further discussion on the notebook: #88

cdeil · 2016-11-30T21:49:12Z

I've split out the discussion / work on high-level docs re-structuring into a separate issue:
#89

astrofrog · 2016-11-30T21:55:54Z

@cdeil - I implemented your comments. If the tests pass, I think we can merge this and add documentation after.

cdeil · 2016-11-30T21:56:26Z

For the polygon code, only the polygonal_overlap_single_subpixel docstring hints at the method that's implemented.

@astrofrog - If you know which algorithm is implemented in the polygon functions, could you mention it by name and link e.g. to wikipedia?

I mean you are linking to https://github.com/gregvw/pnpoly/blob/master/pnpoly.pyx and https://www.ecse.rpi.edu/Homepages/wrf/Research/Short_Notes/pnpoly.html, but from looking at the code and those pages for 1 minute it's not clear to me which algorithm we have.

Is it one of the algorithms mentioned here?
https://en.wikipedia.org/wiki/Point_in_polygon

cdeil · 2016-11-30T21:57:39Z

+1 to merge and add more tests / docs in the coming days.

astrofrog · 2016-11-30T22:28:14Z

@cdeil - the algorithm is the even-odd rule

astrofrog added 2 commits November 30, 2016 14:05

Added initial implementation of polygon_overlap_grid

91c56b7

Explicitly convert vertices to float

12df57b

cdeil added the enhancement label Nov 30, 2016

cdeil added this to the 0.2 milestone Nov 30, 2016

cdeil self-assigned this Nov 30, 2016

Implement contains for polygons

1063e9f

Added URLs

8b232cf

cdeil reviewed Nov 30, 2016

View reviewed changes

Deal properly with scalar PixCoords

bf3f349

cdeil reviewed Nov 30, 2016

View reviewed changes

Add license line

d90426d

astrofrog force-pushed the polygon branch from 294fcf7 to d90426d Compare November 30, 2016 21:24

Add note about why pxd files are required

9f542c1

cdeil suggested changes Nov 30, 2016

View reviewed changes

cdeil mentioned this pull request Nov 30, 2016

Write a regions notebook in astropy-tutorials #88

Open

Tidy up Cython code

b085a2a

cdeil mentioned this pull request Nov 30, 2016

Split up getting started into multiple pages? #89

Closed

astrofrog added 2 commits November 30, 2016 21:52

Added mask reference test for polygon region

c6cef1e

Remove unused import

4f7a97f

cdeil approved these changes Nov 30, 2016

View reviewed changes

Add note about algorithm

e02710d

astrofrog merged commit f456f49 into astropy:master Nov 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial implementation of Polygon to_mask and as_patch #87

Added initial implementation of Polygon to_mask and as_patch #87

astrofrog commented Nov 30, 2016 •

edited

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016 •

edited

astrofrog commented Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

cdeil Nov 30, 2016

astrofrog Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil Nov 30, 2016

astrofrog Nov 30, 2016

astrofrog Nov 30, 2016

cdeil left a comment

cdeil Nov 30, 2016

astrofrog Nov 30, 2016

cdeil Nov 30, 2016

astrofrog Nov 30, 2016 •

edited

cdeil Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016 •

edited

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

Added initial implementation of Polygon to_mask and as_patch #87

Added initial implementation of Polygon to_mask and as_patch #87

Conversation

astrofrog commented Nov 30, 2016 • edited

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016 • edited

astrofrog commented Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog Nov 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016 • edited

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

cdeil commented Nov 30, 2016

cdeil commented Nov 30, 2016

astrofrog commented Nov 30, 2016

astrofrog commented Nov 30, 2016 •

edited

astrofrog commented Nov 30, 2016 •

edited

astrofrog Nov 30, 2016 •

edited

astrofrog commented Nov 30, 2016 •

edited