[WIP] Use GeometryOpsCore for real #223
Conversation
|
What is the status on this? I thought about getting started with adding some basic spherical geometry operations like distance and centroid, but maybe I should wait for these imports to be resolved? |
|
I'm hoping to get this resolved next week feiw, pretty sure it's just a bad kwarg I have to track down |
|
Phew! Things are finally working. |
| @@ -3,7 +3,7 @@ module GeometryOpsLibGEOSExt | |||
| import GeometryOps as GO, LibGEOS as LG | |||
| import GeoInterface as GI | |||
|
|
|||
| import GeometryOps: GEOS, enforce | |||
| import GeometryOps: GEOS, enforce, _True, _False, _booltype | |||
There was a problem hiding this comment.
Feels like we should remove the underscores if we are importing these?
There was a problem hiding this comment.
Good point. We can probably make these uppercase, but only exported from Core (not GeometryOps proper)?
There was a problem hiding this comment.
Just realised we can use all of these in Rasters.jl too as its all the same there already. So no underscores is good.
Really keen to unify GeometryOps/Rasters around this core for all geometry related things now.
| import GeometryOpsCore: | ||
| TraitTarget, | ||
| Manifold, Planar, Spherical, Geodesic, | ||
| BoolsAsTypes, _True, _False, _booltype, |
There was a problem hiding this comment.
Again the underscores on imported objects...
| function segmentize(method::Manifold, geom; max_distance, threaded::Union{Bool, BoolsAsTypes} = _False()) | ||
| @assert max_distance > 0 "`max_distance` should be positive and nonzero! Found $(method.max_distance)." | ||
| _segmentize_function(geom) = _segmentize(method, geom, GI.trait(geom); max_distance) | ||
| return apply(_segmentize_function, TraitTarget(GI.LinearRingTrait(), GI.LineStringTrait()), geom; threaded) |
There was a problem hiding this comment.
Seems like code duplication with the function below, are both needed?
Are these asserts not checked twice when called from the other method?
(Also throwing an error is better than asserts as it's guaranteed to actually run)
| function segmentize(method::SegmentizeMethod, geom; threaded::Union{Bool, BoolsAsTypes} = _False()) | ||
| @warn "`segmentize(method::$(typeof(method)), geom) is deprecated; use `segmentize($(method isa LinearSegments ? "Planar()" : "Geodesic()"), geom; max_distance, threaded) instead!" maxlog=3 | ||
| @assert method.max_distance > 0 "`max_distance` should be positive and nonzero! Found $(method.max_distance)." |
There was a problem hiding this comment.
The same assert is applied again above
This PR switches GeometryOps to actually import the released version of GeometryOpsCore.
It turns out that this works on 1.10, but fails on nightly because it imports anonymous functions as well, and
gensymdoesn't know that so generates identical symbols for different anonymous functions in GeometryOps and GeometryOpsCore. I fix that by creating a static import list of things that GeometryOpsCore defines, are unexported, but must be defined in GeometryOps.Additionally, the PR will deprecate the GeodesicSegments and LinearSegments methods for
GeodesicandPlanarmanifolds. This is in preparation for the next breaking release where we'll introduce perimeter calculations, arclength interpolation, and basic spherical geometry utilities.