Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: more thorough testing of overlay #343

Merged
merged 7 commits into from Apr 30, 2018

Conversation

jorisvandenbossche
Copy link
Member

Added some more tests for overlay comparing to actual output (so using simple example dataframe like in docs, and not only checking shape).
Wanted to do this before #338 to ensure the perf improvements do not change the output.

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Jun 9, 2016

Hmm, something strange going on here. All tests pass locally for me (on Windows), and I also used the exact same example dataframe as in the docs: http://geopandas.readthedocs.io/en/master/set_operations.html#the-different-overlay-operations

For example, the how='intersection gives you a result with three rows (locally, and in the docs as well), but on travis the result has 4 rows. I printed the expected and actual result for debugging in the travis logs, and I get:

# expected (what I hardcoded that it should be)
   col1  col2                             geometry
0     1     1  POLYGON ((2 1, 1 1, 1 2, 2 2, 2 1))
1     2     1  POLYGON ((3 3, 3 2, 2 2, 2 3, 3 3))
2     2     2  POLYGON ((3 4, 4 4, 4 3, 3 3, 3 4))

# actual result on travis
   col1  col2                                       geometry
0     1     1            POLYGON ((2 1, 1 1, 1 2, 2 2, 2 1))
1     2     1            POLYGON ((3 3, 3 2, 2 2, 2 3, 3 3))
2     2     2  POLYGON ((4 3, 4 4, 3 4, 3 5, 5 5, 5 3, 4 3))
3     2     2            POLYGON ((3 4, 4 4, 4 3, 3 3, 3 4))

The reproducible example from the tests/docs is:

from geopandas import GeoSeries, GeoDataFrame
from shapely.geometry import Polygon

s1 = GeoSeries([Polygon([(0,0), (2,0), (2,2), (0,2)]),
                Polygon([(2,2), (4,2), (4,4), (2,4)])])
s2 = GeoSeries([Polygon([(1,1), (3,1), (3,3), (1,3)]),
                Polygon([(3,3), (5,3), (5,5), (3,5)])])

df1 = GeoDataFrame({'geometry': s1, 'col1':[1,2]})
df2 = GeoDataFrame({'geometry': s2, 'col2':[1,2]})

geopandas.overlay(df1, df2, how='intersection')

You can see in the result on travis that the coordinates even include '5', while the coordinates in df1 only go up to '4', so this should never be the result for an intersection.

Does anybody see what I am missing?

@kjordahl @perrygeo

@jorisvandenbossche jorisvandenbossche force-pushed the overlay-tests branch 2 times, most recently from b53dcb8 to 335b2a3 Compare June 10, 2016 13:44
@jorisvandenbossche
Copy link
Member Author

Short report of what I discovered untill now: the reason for the difference lies in the use of representative_point. For the example Polygon used in the tests, this returns something else for me locally as on travis. And, on travis it gives a point that is located at the boundary, and therefore, the intersects with a neighbouring polygon (that only shares that boundary) gives True, while we should get False for overlay.
I tried to replace intersects with overlaps and within/contains, but no succes for now (they do not give the exact behaviour we want)

@nickeubank
Copy link
Contributor

@jorisvandenbossche maybe the better place to tweak is at the creation of the representative point -- line 116 in overlay.py.

Can you tweak with something like:

    cent = newpoly.representative_point()
    if not newpoly.contains(cent):
        newpoly = newpoly.buffer(-1e-10)
        cent = newpoly.representative_point()

Trying to think if there's a parameter that would set the right value for the negative buffer. Maybe we can add a parameter to overlay of resolution to determine that, and pass that to buffer?

Revert "BUG: overlay: change intersects to within"

This reverts commit e7133b6.

Revert "temp add ozak's work"

This reverts commit 02d4729.
@jorisvandenbossche jorisvandenbossche merged commit ce8f70c into geopandas:master Apr 30, 2018
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.

None yet

2 participants