flopy icon indicating copy to clipboard operation
flopy copied to clipboard

bug: GridIntersect improvements

Open dbrakenhoff opened this issue 1 year ago • 5 comments

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?
  • [x] 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 avatar Oct 22 '24 14:10 dbrakenhoff

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.

christianlangevin avatar Oct 22 '24 20:10 christianlangevin

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.

mwtoews avatar Oct 22 '24 21:10 mwtoews

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 avatar Oct 23 '24 08:10 dbrakenhoff

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

wpbonelli avatar Oct 28 '24 22:10 wpbonelli

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

dbrakenhoff avatar Oct 29 '24 13:10 dbrakenhoff