bug: GridIntersect improvements

modflowpy / flopy

A Python package to create, run, and post-process MODFLOW-based models.

https://flopy.readthedocs.io

Other

522 stars 314 forks source link

bug: GridIntersect improvements #2344

Open dbrakenhoff opened 1 month ago

dbrakenhoff commented 1 month ago

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

[x] 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 #2342 (for "vertex" mode, which was fixed in #2343).
[x] Return intersection result as GeoDataFrame (instead of np.recarray) if geopandas is available.
[x] 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!

langevin-usgs commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

wpbonelli commented 3 weeks ago

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

dbrakenhoff commented 3 weeks ago

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