ENH: Add clip module by lwasser · Pull Request #1128 · geopandas/geopandas

lwasser · 2019-09-16T18:58:40Z

Addressing: #821

This PR is the beginning of adding the clip module to geopandas. As i began to copy things over, i noticed that the data used in our vignette uses data downloaded with an earthpy function. So I will need to refactor it to work here. @nkorinek is actually working on that here:

earthlab/earthpy#414

So this is also a WIP PR. so far tests for this module are working just fine but I want to see what CI has to say as well. I noticed some of the other tests are failing locally but they have nothing to do with what i am adding here. Excited to see this functionality in GeoPandas!!
This pr complements #1127 where i'm updating docs as I contribute.

please say the word if something needs to be changed, fixed, etc!!

Closes #956, closes #821

updating from geopandas master

Test fiona 1.8b1 (geopandas#844)

Update from geopandas base

codecov · 2019-09-16T21:48:13Z

Codecov Report

Merging #1128 into master will decrease coverage by 0.06%.
The diff coverage is 88.05%.

@@            Coverage Diff            @@
##           master   #1128      +/-   ##
=========================================
- Coverage   90.06%     90%   -0.07%     
=========================================
  Files          21      22       +1     
  Lines        2094    2161      +67     
=========================================
+ Hits         1886    1945      +59     
- Misses        208     216       +8

Impacted Files	Coverage Δ
geopandas/__init__.py	`100% <100%> (ø)`	⬆️
geopandas/tools/__init__.py	`100% <100%> (ø)`	⬆️
geopandas/tools/clip.py	`87.69% <87.69%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca6e5e7...6e7b462. Read the comment docs.

martinfleis

It looks good! Looking forward having it implemented. I have made some comments, but nothing critical. I haven't checked tests so far, but that can wait once we'll know what exactly should be tested.
Thank you for moving this from earthpy to geopandas!

lwasser · 2019-09-16T23:33:21Z

thank you @martinfleis i left a question in the issue as well. i just am not sure what the review process is here for geopandas! i am still working on some cleanup but should i go ahead and address your comments first? will there be others that need to review and so I should wait for that input as well? Or just dig in to these first and see if other things come up? thank you!

martinfleis · 2019-09-17T07:51:22Z

@lwasser It is highly probable that @jorisvandenbossche will leave his comments as well, so you can wait with any changes until then. You can respond to comments where we can expect some kind of discussion though. Or just let us know your thoughts on the comments (they are suggestions, not rock solid requests).

jorisvandenbossche · 2019-09-17T20:57:15Z

@lwasser you can certainly already start replying to Martin's comments, or updating for comments that seem straightforward. Will try to give this a more detailed look tomorrow. In any case, already thanks for starting the PR!

jorisvandenbossche

@lwasser already added a few questions about the implementation

ResidentMario · 2019-09-29T21:10:30Z

Nice to see this functionality being worked on. I implemented a clip operation in the geoplot codebase that does similar work.

lwasser · 2019-09-30T19:20:12Z

thanks all!! i know i have some more work to do on this PR. i've been slammed this semester but will work on this more THIS WEEK.

use rtree for points too! Co-Authored-By: Martin Fleischmann <36797143+martinfleis@users.noreply.github.com>

Added changes requested by geopandas to clip and vignette

lwasser · 2019-11-14T17:18:10Z

hi again @martinfleis and @jorisvandenbossche can you please have another look at this PR? we've spent some time (@nkorinek and myself) working on your comments. my apologies for the slowness of our effort. we are juggling a bunch of different projects right now!!

jorisvandenbossche · 2019-11-14T21:38:21Z

@lwasser thanks a lot for the update! I will also take a look tomorrow

lwasser · 2019-11-14T21:45:03Z

wonderful @jorisvandenbossche ! we will have that CI issue fixed by then as well. it's just a black format issue and i just need to push the commit through!!

lwasser · 2020-01-17T20:43:13Z

hey there @martinfleis @jorisvandenbossche just checking in on this PR and the time frame for when it might be merged! if there are things that we missed please do say the word.

jorisvandenbossche · 2020-01-17T21:30:59Z

Hi @lwasser, I hope to finally spend some time again on my geopandas backlog after the holidays the coming days!

jorisvandenbossche

OK, sorry for the slow follow-up, but this is looking really good!
I did a pass over the code myself and pushed some clean-ups (which would otherwise be tedious to put as comments), and left a few remaining inline comments / questions.

For the tests, I think it would be good to have at least one test where we actually compare with a known result with intersected geometries (with assert_geodataframe_equal, now most tests only check the types of the output etc). This can be a small test where the result is can be easily constructed manually as well.
Also a test that ensures that the output still has the same crs as the input would be good.

lwasser · 2020-02-04T22:24:21Z

hi team! @martinfleis @ljwolf @jorisvandenbossche i just wanted to check in. i'm currently fairly busy as i'm teaching this semester and working on some tools to support our class!! i will try to have a look at the comments left on this pr. it may however take me some time!!

I'm curious if there are things that you guys would like that you could just add vs things you'd like for me or @nkorinek to work on?

i'm just trying to be sensitive to my time but also i want to see this merged so we can deprecate it from earthpy as there are bugs over there that we've fixed here! Please let me know what you think! This pr has been open for a while so i hadn't expected to be working on it this spring. i'm not complaining as i just want the best for this package and to help where i can. i'm also just short on time right now.
Cheers!

martinfleis

@lwasser sorry it takes so long. I'll take care of latest comments in the next couple of days.

lwasser · 2020-02-04T23:06:12Z

@martinfleis thank you! and please don't apologize! i really understand how much effort it is to maintain these projects and appreciate all of the work. Thank you for working on some of the comments! please let us know where we can chip in. like i said i'm really excited about this being incorporated into geopandas! i will then update all of our lessons accordingly.

martinfleis · 2020-02-05T10:46:39Z

I have made changes as per comments, to support GeometryCollections, cleaned warnings and conditions for keep_geom_type to do the check only if needed, changed clip_obj to mask (same as in GDAL geoprocessing tools in QGIS) and changed overlap check.

I still want to do couple more tests later today or tomorrow.

lwasser · 2020-02-05T18:33:23Z

thank you @martinfleis !!

jorisvandenbossche

@martinfleis thanks for the updates!

martinfleis · 2020-02-06T23:36:56Z

There are no remaining unresolved comments, some tests are using assert_geodataframe_equal and code coverage is 100%. And we are all green.

@jorisvandenbossche shall we merge?

jorisvandenbossche · 2020-02-07T14:38:42Z

@jorisvandenbossche shall we merge?

Yes, let's do that :)

jorisvandenbossche · 2020-02-07T14:42:39Z

@lwasser @nkorinek @martinfleis @ljwolf thanks all for the nice collaborative PR! (and for your patience .. :))

lwasser · 2020-02-07T19:10:57Z

@jorisvandenbossche @martinfleis @ljwolf and everyone!! Wow thank you so much for this!! I'm so excited that this is now merged and also excited that we were able to move clip over here where it belongs!! Just curious... when will the next release of geopandas be? I'll then plan to update our online lessons and deprecated clip from earthpy.

jorisvandenbossche · 2020-02-07T19:36:24Z

when will the next release of geopandas be

-> #1285

Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net> Co-authored-by: Nathan Korinek <nako1890@colorado.edu> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Leah Wasser added 8 commits September 18, 2018 09:20

Merge pull request #1 from geopandas/master

ff46c60

updating from geopandas master

Merge pull request #2 from geopandas/master

4424c1f

updating from geopandas master

Fixing minor misspelling in Contributing.md file

fe46077

Merge pull request #3 from geopandas/master

2c87951

updating from geopandas master

Merge pull request #4 from geopandas/master

41e9ffc

Test fiona 1.8b1 (geopandas#844)

Merge pull request #5 from geopandas/master

a0a27df

Update from geopandas base

adding clip module to geopandas from earthpy

453dd0d

lint all files

9ad9fb9

martinfleis reviewed Sep 16, 2019

View reviewed changes

jorisvandenbossche reviewed Sep 18, 2019

View reviewed changes

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Comment thread geopandas/clip.py Outdated

Leah Wasser and others added 4 commits September 30, 2019 13:38

update docstrings - clip_gdf more expressive

600c005

rename clip_shp --> clip

2782f36

Update geopandas/clip.py

c548c49

use rtree for points too! Co-Authored-By: Martin Fleischmann <36797143+martinfleis@users.noreply.github.com>

black

87170d7

lwasser mentioned this pull request Oct 8, 2019

EarthPy: Software Submission for Review pyOpenSci/software-submission#3

Closed

21 tasks

lwasser mentioned this pull request Oct 25, 2019

Reconsider exceptions raised in validating clip_shp args earthlab/earthpy#437

Closed

nkorinek and others added 4 commits November 7, 2019 16:34

Added changes requested by geopandas to clip

5e294fd

Fixed the example data in the example inside the clip function.

6a67eaf

Made changes suggested on GitHub to improve some wording issues

bc2d1d3

Merge pull request #6 from nkorinek/add-clip

bfac14c

Added changes requested by geopandas to clip and vignette

allow Polygon, bounds check, allow mixed geometry

0197947

martinfleis changed the title ~~Add clip module to geopandas~~ ENH: Add clip module Dec 19, 2019

jorisvandenbossche added 3 commits January 20, 2020 16:14

Merge remote-tracking branch 'upstream/master' into add-clip

ffa0f41

move test file

4b96e8e

some general clean-up

0cbb14c

jorisvandenbossche reviewed Jan 21, 2020

View reviewed changes

Comment thread geopandas/tools/clip.py

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/tests/test_clip.py Outdated

ljwolf reviewed Jan 24, 2020

View reviewed changes

Comment thread geopandas/tools/clip.py Outdated

martinfleis reviewed Feb 4, 2020

View reviewed changes

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/clip.py

martinfleis added 3 commits February 4, 2020 23:18

bounds overlap check

9824975

cleanup, clip_obj -> mask

68bcde6

GeometryCollection support

19b0281

jorisvandenbossche reviewed Feb 5, 2020

View reviewed changes

Comment thread geopandas/tools/clip.py Outdated

Comment thread geopandas/tools/clip.py Outdated

martinfleis added 3 commits February 5, 2020 22:03

minor changes as per comments

121cc08

tests

ec4ff99

LinearRing test

a33bb05

jorisvandenbossche added 2 commits February 7, 2020 10:50

Merge remote-tracking branch 'upstream/master' into add-clip

b290394

sphinx gallery thumbnail

2e87628

jorisvandenbossche merged commit 0dde1c7 into geopandas:master Feb 7, 2020

lwasser mentioned this pull request Feb 7, 2020

Functions to Clip Point, Line and Polygon Data Like Those that exist in other spatial GIS tools #821

Closed

Uh oh!

Uh oh!

Conversation

lwasser commented Sep 16, 2019 • edited by martinfleis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

martinfleis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lwasser commented Sep 16, 2019

Uh oh!

martinfleis commented Sep 17, 2019

Uh oh!

jorisvandenbossche commented Sep 17, 2019

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ResidentMario commented Sep 29, 2019

Uh oh!

lwasser commented Sep 30, 2019

Uh oh!

lwasser commented Nov 14, 2019

Uh oh!

jorisvandenbossche commented Nov 14, 2019

Uh oh!

lwasser commented Nov 14, 2019

Uh oh!

lwasser commented Jan 17, 2020

Uh oh!

jorisvandenbossche commented Jan 17, 2020

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lwasser commented Feb 4, 2020

Uh oh!

martinfleis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lwasser commented Feb 4, 2020

Uh oh!

martinfleis commented Feb 5, 2020

Uh oh!

lwasser commented Feb 5, 2020

Uh oh!

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lwasser commented Sep 16, 2019 •

edited by martinfleis

Loading

codecov Bot commented Sep 16, 2019 •

edited

Loading