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

feat(Gridintersect): new grid intersection options #1468

Merged
merged 25 commits into from
Jul 27, 2022

Conversation

dbrakenhoff
Copy link
Contributor

Features:

Bug fixes:

Other improvements:

jdhughes-usgs and others added 25 commits June 11, 2020 11:26
ensure one entry per grid cell in polygon intersection result
add test for this case
bug in structured mode where linestrings were not handled
correctly if they pass in and out of grid cell
- fix vertices when intersecting linestring with offset grids
allow intersection shape to be passed as list of vertices (modify test to test this)
specify keyword arguments explicitly in intersect calls
formatting
- contains_centroid
- min_area_fraction
add tests
- add southeast diagonal neighbor in search if other neighbors produce no intersections
- maintain intersection calculation order in result
- add test
- add return_all_intersection options for points and linestrings on boundaries
- improve plot_polygon method
- black formatting and sort imports
- add tests for return_all_intersections
- parse result to only include polygons in _intersect_polygon_structured
@dbrakenhoff
Copy link
Contributor Author

I also wanted to deal with the shapely DeprecationWarning "ShapelyDeprecationWarning: Setting custom attributes on geometry objects is deprecated, and will raise an AttributeError in Shapely 2.0".

But I was wondering what the policy is on backward compatibility for optional dependencies?

The new methods needed to silence this warning are available from shapely 1.8.0 onwards. Do we continue supporting shapely<1.8 for a while, or can we break the code for older versions of shapely?

Let me know, and i will commit a fix accordingly.

@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #1468 (6746000) into develop (1757470) will decrease coverage by 44.926%.
The diff coverage is 3.973%.

@@              Coverage Diff               @@
##           develop     #1468        +/-   ##
==============================================
- Coverage   74.970%   30.044%   -44.927%     
==============================================
  Files          249       249                
  Lines        53645     52276      -1369     
==============================================
- Hits         40218     15706     -24512     
- Misses       13427     36570     +23143     
Impacted Files Coverage Δ
flopy/utils/gridintersect.py 7.455% <3.973%> (-75.677%) ⬇️
flopy/mf6/utils/testutils.py 0.000% <0.000%> (-93.334%) ⬇️
flopy/mfusg/mfusgsms.py 6.190% <0.000%> (-91.941%) ⬇️
flopy/mf6/utils/postprocessing.py 9.259% <0.000%> (-88.889%) ⬇️
flopy/modflow/mfevt.py 7.537% <0.000%> (-87.463%) ⬇️
flopy/utils/mtlistfile.py 4.620% <0.000%> (-85.348%) ⬇️
flopy/mf6/utils/lakpak_utils.py 2.097% <0.000%> (-85.315%) ⬇️
flopy/modflow/mfhyd.py 13.223% <0.000%> (-85.124%) ⬇️
flopy/utils/swroutputfile.py 10.714% <0.000%> (-85.079%) ⬇️
flopy/modflowlgr/mflgr.py 6.271% <0.000%> (-84.040%) ⬇️
... and 206 more

Copy link
Contributor

@jlarsen-usgs jlarsen-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good overall. The only question I have is about the

sys.path.insert(1, "..")

call in t065_test_gridintersect.py. Is there a support script that is located within that path that is not explicitly installed into python?

Copy link
Contributor

@jlarsen-usgs jlarsen-usgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR. @w-bonelli is going to remove the sys.path.insert() call from t065 in his upcoming PR.

@dbrakenhoff
Copy link
Contributor Author

Sorry, the insert was just to get the code to use my fork instead of the installed flopy package. Thanks for dealing with that!

Any advice on dealing with that shapely deprecation warning?

@jdhughes-usgs jdhughes-usgs merged commit 4f86fcf into modflowpy:develop Jul 27, 2022
@jlarsen-usgs
Copy link
Contributor

@dbrakenhoff

It looks like we won't be able to use p.name, which is used to store the cellid, once shapely 2.0.0 is released. Would it break anything or cause performance issues to instead store a dictionary of {name : shapely geometry object} and then update the internals to work with a convention like this instead of storing the cellid directly in the object?

@dbrakenhoff
Copy link
Contributor Author

That would certainly be an option, but not sure if that would hurt performance.

I'd prefer to go the method Shapely 2.0 suggests:

geoms, items = self._get_gridshapes()  # modify method to return geoms and cellids
self.strtree = strtree.STRtree(geoms, items)
# then get either geoms or cellids internally with: 
self.strtree.query_geoms()
self.strtree.query_items()

Only trouble is then we'd need to add logic for Shapely < 1.8 users if that is something we still want to support. I'd be okay with requiring shapely > 1.8 since it's an optional dependency anyway, but not sure how you guys think about that.

PS. this is probably something for a new issue.

@langevin-usgs
Copy link
Contributor

I support requiring shapely > 1.8 unless it wasn't available for one of our supported operating systems. It looks like win, Mac, and linux are all supported.

@jdhughes-usgs
Copy link
Contributor

@dbrakenhoff I would say it would be fine to support shapely > 1.8 now in develop and the in the next flopy release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants