[WIP] Use GeometryOpsCore for real #223
Conversation
src/GeometryOps.jl
Outdated
| import .GeometryOpsCore | ||
| for name in setdiff(names(GeometryOpsCore, all = true), (:eval, :var"#eval", :include, :var"#include")) | ||
| import GeometryOpsCore | ||
| for name in Base.setdiff( |
There was a problem hiding this comment.
This is pretty obscure, is there a less scary way to do this so regular contributors don't run away straight away?
Personally I would just make a single tuple with everything you want to import and skip the union/setdiff part.
We should also export everything explicitly so people can read it
(to me this file is mostly about communicating an overview of the package clearly to other devs)
There was a problem hiding this comment.
Ah yeah that's a good point. I can probably just explicitly import stuff.
|
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. |
| @@ -51,6 +51,8 @@ end | |||
|
|
|||
| A spherical manifold means that the geometry is on the 3-sphere (but is represented by 2-D longitude and latitude). | |||
|
|
|||
| By default, the radius is defined as the mean radius of the Earth, `6371008.8 m`. | |||
There was a problem hiding this comment.
Can we make global named constants for all the numbers in these files, and both insert them in docs and use them for keywords?
Maybe in a different PR, just commenting because this text is added here
| @@ -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
GeodesicandLinearmanifolds. This is in preparation for the next breaking release where we'll introduce perimeter calculations, arclength interpolation, and basic spherical geometry utilities.