Implement a GEOS algorithm in an extension

[asinghvi17]

This PR implements a parametrized GEOS algorithm within GeometryOps, and factors out the integration work to an extension GeometryOpsLibGEOSExt. (cf #76)

The extension is currently implemented for polygon set ops, DE-9IM functions, and buffering.

With this, you basically pass GEOS(; params...) as the first argument to whichever functions have been overridden. Ideally, this should be at least most of the available functions.

GO.buffer(GEOS(), p1, 1)

Needs docs and tests.

[asinghvi17]

I will have to work on this tomorrow but a basic framework of what has to happen:

• GEOS has to not be typed at all
• get_defaults(geos, kwargs_input; defaults...) or something that returns a namedtuple of defaults from the given kwargs.
• Potentially do misspelling checks, but that's not the goal here.

Sidenote: we need topological simplify as well from GEOS, that can be its own object.

[rafaqz]

GEOS has to not be typed at all

Can you explain this a bit

[asinghvi17]

The parameter in the type can't exist, or more accurately it shouldn't be used for dispatch (which removes its purpose altogether)

[rafaqz]

Well dispatch not the only purpose for type parameters... My concern would be where we are calling LibGEOS deep in apply and type instability in the repeated calls will matter quite a bit.

[asinghvi17]

I guess that brings up another topic - should we eagerly convert back to GI.* or leave geometries as LibGEOS geoms?

[rafaqz]

Its almost an arbitrary descision, we may need real world use to know either way. Eager conversion is clean and will make subsequent native julia ops faster, but always makes the GEOS op slower. And e.g. if people do repeated GEOS operations not converting will be fastest overall.

Like your issue over at Rasters is caused by an unused eager conversion step in GDAL:
rafaqz/Rasters.jl#634

If GDAL could just pass points through to Proj unchanged there would be no problem.

Having GeoInterface we can avoid that. But I'm not sure its worth it on balance. Maybe a keyword can control it eventually, and users can choose not to convert back when they need the performance for something.

[asinghvi17]

It does make things like segmentize type unstable unless you explicitly pass a method which is a bit strange...

[asinghvi17]

@rafaqz this is what I was thinking of - in any case kwargs will not change either the function called or return type, so the purpose they serve is limited.

The only controlling factor might be target, but that's not a parameter to the GEOS object in any case.

[rafaqz]

Ok if they dont affect types its less an issue. I guess just put LibGEOS calls in explicit conditionals to avoid dyamic dispatch slowness.

[rafaqz]

Some tests would be nice ;)

Also just to see what everything actually does.

[asinghvi17]

Seems fair. I'm not going to test too thoroughly here since LibGEOS should be taking care of that, but can definitely test type combos + return types and keyword defaults/enforcement.

[rafaqz]

At least make sure everything actually runs ;)

[asinghvi17]

The PR should be ready for review now!

[asinghvi17]

Oh geez...test failure is not present on my machine. I guess allowing Fix1s to take arbitrary arguments was a 1.10 thing.

Will modify the tests accordingly.

[skygering]

Its almost an arbitrary descision, we may need real world use to know either way. Eager conversion is clean and will make subsequent native julia ops faster, but always makes the GEOS op slower. And e.g. if people do repeated GEOS operations not converting will be fastest overall.

Like your issue over at Rasters is caused by an unused eager conversion step in GDAL: rafaqz/Rasters.jl#634

If GDAL could just pass points through to Proj unchanged there would be no problem.

Having GeoInterface we can avoid that. But I'm not sure its worth it on balance. Maybe a keyword can control it eventually, and users can choose not to convert back when they need the performance for something.

I like the idea of a keyword eventually. It really does depend on the use case and it is nice to have control over your types.

[asinghvi17]

Maybe we could change the types at some point to have a GEOSDirect and GEOSConvert algorithm, where the results from LG functions go into postprocess_results(::GEOSDirect, ...) = identity and postprocess_results(::GEOSConvert, ...) = GO.tuples(...) or something.

[asinghvi17]

The geometry correction seems a bit difficult to do correctly, so I'm considering removing it from this PR and fixing this up to merge as is.

[asinghvi17]

I've had to add a hack in the test file for LG geometrycollection conversion to work, but all tests pass locally, and are using the GEOS backend for GeometryOps rather than LibGEOS directly.

IMO this should be ready for merge if tests pass.

[skygering]

Looks good overall! Just a few clarifying questions to address.

[rafaqz]

LGTM