stop using Base.convert

[visr]

Following discussion in #49.

I tried implementing Base.convert as suggested by the GeoInterface docs for GeometryBasics, testing with GeoJSON geometries.

The first method is needed to send the 2-arg convert through the 3-arg method with the trait argument.

function Base.convert(::Type{T}, geom) where {T<:AbstractGeometry}
    return convert(T, GeoInterface.geomtrait(geom), geom)
end

This method is geom::Any, so if there are any convert methods with more specific types already defined, I cannot hit this anymore. This was the case with trying to convert(GeometryBasics.Point, p::GeoJSON.Point). GeoJSON.Point is an AbstractVector, and there is a method defined for those already. However that method doesn't work (since the number of dimensions, 2 or 3, is not statically known).

So here I removed all Base.convert usages from docs and examples. There is already a GeoInterface.convert method

convert(T, geom) = convert(T, geomtrait(geom), geom)

That works well, and I figured we don't need to ask developers to add a fast passthrough if we can define it ourselves in fallback.jl:

# no conversion needed
convert(::Type{T}, ::AbstractTrait, x::T) where {T} = x

[visr]

Probably it would be better to move the "no conversion needed" next to the 2 to 3 arg method, and make it 2 arg, avoiding the geomtrait call.

[evetion]

@rafaqz and I discussed this in the past and we settled for using Base.convert. While harder to implement, it enables more automatic conversions and it users know it already.

In terms of quick fallback, for LibGEOS, I did the following: https://github.com/JuliaGeo/LibGEOS.jl/commit/b6902e14251abaf949887c3898f4687818299374.

[evetion]

I haven't tested the GeometryBasics code yet, but in your example:

This method is geom::Any, so if there are any convert methods with more specific types already defined, I cannot hit this anymore. This was the case with trying to convert(GeometryBasics.Point, p::GeoJSON.Point). GeoJSON.Point is an AbstractVector, and there is a method defined for those already. However that method doesn't work (since the number of dimensions, 2 or 3, is not statically known).

The conversion would still work for any non GeoJSON.Point geometries right? Also, the GeoJSON conversion, if defined, could be removed in favour of the generic GeoInterface conversion?

[visr]

The conversion would still work for any non GeoJSON.Point geometries right?

No, it would only work for geometries for which Any is the most specific method it hits, which is something that cannot be relied on.

Also, the GeoJSON conversion, if defined, could be removed in favour of the generic GeoInterface conversion?

I'm talking about the generic GeoInterface conversion, which is the only one I implemented. I was testing using GeoJSON as source geometries.

[visr]

So does that mean that this function in this package:

convert(T, geom) = convert(T, geomtrait(geom), geom)

Accidentally defined GeoInterface.convert and was supposed to have been Base.convert(T, geom) = ...?

While harder to implement, it enables more automatic conversions and it users know it already.

For those reasons I would've also preferred Base.convert if it could work in general. But this interface is built around traits, and not around subtyping. So it would be strange if for this one function it would depend on subtyping instead. We want to be able to support geometry types like:

struct LineString <: AbstractVector{Float64}
    # [p1.x p1.y p2.x p2.y ...]
    coords::Vector{Float64}
end

right? For types like this it can be useful to make them an AbstractVector.

[rafaqz]

Yeah, its a bit annoying because of the manual definitions required.

But Base.convert is a bit of a special case. Its the generic way of converting things in Julia, and Base calls it in places, like building structs and assinging to arrays, function return types etc.

I think thats nice to have for all geometry types?

convert here only gives us manual conversion.

[evetion]

Accidentally defined GeoInterface.convert and was supposed to have been Base.convert(T, geom) = ...?

Sorta. I started out with convert and forgot to change all references to Base.convert.

[jw3126]

I think it would be great to make a decision here. Personally, I am not in favor of overloading Base.convert, in addition to the reasons mentioned above such overloads can create ambiguities in my experience. One ecosystem creates a Base.convert(::Type{Something}, ::Any), another creates Base.convert(::Type{<:Any}, ::SomethingElse) and if you combine both you get ambiguities. See for instance https://github.com/JuliaInterop/CxxWrap.jl/issues/313#issuecomment-1314967238 for a recent example.

[rafaqz]

Hmm I'm tending to agree with you ans @visr now, maybe GeoInterface.convert is better.

[evetion]

Agreed, and maybe also geoplot (like graphplot for graphs) should make things way easier as well.

[rafaqz]

Ok. well lets put in some time to get GeoInterface.convert working everywhere, then think about package level conversions.

I don't think we can delete the Base.convert methods here though, probably we have to leave them but switch documentation to GeoInterface.convert.

[visr]

I don't think we can delete the Base.convert methods here though, probably we have to leave them but switch documentation to GeoInterface.convert.

Why not? There is only one method add to Base.convert, which always throws an error:

Base.convert(T::Type, ::AbstractGeometryTrait, geom) = error("..")

If we remove it, calling this methods will still throw an error, just a MethodError instead of an ErrorException.