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.

Sign up for GitHub

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

Jump to bottom

bug: GridIntersect improvements #2344

Open
3 of 4 tasks
dbrakenhoff opened this issue Oct 22, 2024 · 5 comments · Fixed by #2346
Open
3 of 4 tasks

bug: GridIntersect improvements #2344

dbrakenhoff opened this issue Oct 22, 2024 · 5 comments · Fixed by #2346
Assignees
@dbrakenhoff
Labels
bug deprecation maintenance

Comments

@dbrakenhoff
Copy link
Contributor

dbrakenhoff commented Oct 22, 2024 •
edited
Loading

Some things I'd like to fix in gridintersect.py:

  • When intersecting linestrings with GridIntersect in structured mode, and those intersections result in GeometryCollections, there are overlapping results in the intersection output. This was originally reported in bug: typo shapely function call within gridintersect #2342 (for "vertex" mode, which was fixed in fix(gridintersect): typo shapely function call within gridintersect #2342 #2343).
  • Return intersection result as GeoDataFrame (instead of np.recarray) if geopandas is available.
  • Remove shapely<2.0 code. It's been released for a while now so perhaps we can get rid of some old code?
  • Deprecate method="structured" mode? This is somewhat complex to maintain, and performs worse than "vertex" mode in almost all cases, especially since Shapely 2.0 was released. This has the added advantage of "solving" the first issue in this list.

I'm curious to hear your thoughts about these suggestions, so let me know what you think, and I can put together a PR!
@dbrakenhoff dbrakenhoff self-assigned this Oct 22, 2024
@dbrakenhoff dbrakenhoff mentioned this issue Oct 22, 2024
Merged
@langevin-usgs
Copy link
Contributor

Hi @dbrakenhoff, sounds like these are nice fixes. I think it's time to remove the shapely<2.0 code. I wonder if @mwtoews agrees? I'm also good with removing the structured mode. Doesn't seem like it's worth it to maintain. Regarding the GeoDataFrame return, should we add an optional argument to request that instead of returning based on whether or not geopandas is available? I think that could break our tests and examples. I think we will be switching more heavily to geopandas in the future, so maybe that would be a better time to make that type of change.

@mwtoews
Copy link
Contributor

mwtoews commented Oct 22, 2024

I think it's fine to establish a minimum shapely 2.0, as it is widely available and replaces older shapely on all supported platforms. It should make code logic much easier to review. For the record, a key improvement with shapely 2.0 is that numpy arrays of geometries are supported, and processed much more efficiently using vectorized ufuncs written in C.

@dbrakenhoff
Copy link
Contributor Author

Alright, I'll put something together and submit it for review.

Regarding the GeoDataFrame return, should we add an optional argument to request that instead of returning based on whether or not geopandas is available? I think that could break our tests and examples. I think we will be switching more heavily to geopandas in the future, so maybe that would be a better time to make that type of change.

And good point about this, I'll make it an option and we'll see what makes sense as geopandas becomes more integrated with flopy.

@dbrakenhoff dbrakenhoff linked a pull request Oct 23, 2024 that will close this issue
fix(gridintersect): clean up of gridintersect #2346
Merged
wpbonelli pushed a commit that referenced this issue Oct 24, 2024
@dbrakenhoff
refactor(gridintersect): clean up gridintersect (#2346)
f8810c2 
This PR addresses #2344 and does the following:

    * Add DeprecationWarning when method != "vertex"
    * Remove all Shapely<2.0 code
    * Remove warnings filters for older numpy versions
    * Mark methods/tests that should be removed when method="structured" is officially removed
    * Ensure full test suite is maintained after removal of structured tests.
    * Move raster tests to a separate file.
    * Add option to return intersection results as (Geo)DataFrame
    * Allow plotting options to take as input GeoDataFrame (bit unnecessary as they have their own plotting logic) and improve plotting results.
    * Add plot_intersection_result() to plot result + modelgrid (optional).
    * Update example notebook.

Note: Tests for 3D point intersections above/inside grid are deactivated. This is not yet working for method="vertex". Will probably be added in a separate PR.
wpbonelli pushed a commit that referenced this issue Oct 25, 2024
@dbrakenhoff
fix(gridintersect): fix multiple issues (#2343)
34043ab 
Fix #2342:

    * fix typo in shapely.multilinestrings
    * fix issue with np.apply_along_axis
    * consider geometry collections when computing overlaps
    * add test for vertex mode
    * add inactive test for structured mode (not yet working)

Notes:

    * np.apply_along_axis was reducing result from multiple GeometryCollections to a single MultiLineString causing duplication of shapes in the intersection result.
    * structured support will be dropped in future releases (see #2344)
@wpbonelli wpbonelli reopened this Oct 28, 2024
@wpbonelli
Copy link
Member

I think merging those PRs auto-closed this, sorry @dbrakenhoff

@dbrakenhoff
Copy link
Contributor Author

No worries, I guess we leave this open until we can remove the structured method and associated code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dbrakenhoff dbrakenhoff

Labels
bug deprecation maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(gridintersect): clean up of gridintersect dbrakenhoff/flopy
4 participants
@mwtoews @langevin-usgs @dbrakenhoff @wpbonelli