Skip to content

stop using Base.convert #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 1, 2023
Merged

stop using Base.convert #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f329ce5
stop using Base.convert
visr Aug 3, 2022
32df099
move convert no-op to interface.jl
visr Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions docs/src/guides/developer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ And lastly, there are many other optional functions for each specific geometry.
### Conversion
It is useful if others can convert any custom geometry into your
geometry type, if their custom geometry supports GeoInterface as well.
This requires the following three methods, and the last one requires more code to generate `T` with `ngeom`, `getgeom` or just `coordinates` calls.
This requires the following methods, where the implementation should be defined in terms
of GeoInterface methods like `ngeom`, `getgeom`, or just `coordinates` calls.

```julia
Base.convert(::Type{T}, geom) where T<:AbstractPackageType = Base.convert(T, geomtrait(geom), geom)
Base.convert(::Type{T}, ::LineStringTrait, geom::T) = geom # fast fallthrough without conversion
Base.convert(::Type{T}, ::LineStringTrait, geom) = ... # slow custom conversion based on ngeom and getgeom
# This method will get called on GeoInterface.convert(::Type{T}, geom)
# if geomtrait(geom) == LineStringTrait()
GeoInterface.convert(::Type{CustomLineString}, ::LineStringTrait, geom) = ...
GeoInterface.convert(::Type{CustomPolygon}, ::PolygonTrait, geom) = ...
```

## Required for Feature(Collection)s
Expand Down
4 changes: 1 addition & 3 deletions src/fallbacks.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,12 @@ getfeature(t::AbstractFeatureCollectionTrait, fc) = (getfeature(t, fc, i) for i
# Backwards compatibility
coordinates(t::AbstractPointTrait, geom) = collect(getcoord(t, geom))
coordinates(t::AbstractGeometryTrait, geom) = collect(coordinates.(getgeom(t, geom)))
function coordinates(t::AbstractFeatureTrait, feature)
function coordinates(t::AbstractFeatureTrait, feature)
geom = geometry(feature)
isnothing(geom) ? [] : coordinates(geom)
end
coordinates(t::AbstractFeatureCollectionTrait, fc) = [coordinates(f) for f in getfeature(fc)]

Base.convert(T::Type, ::AbstractGeometryTrait, geom) = error("Conversion is enabled for type $T, but not implemented. Please report this issue to the package maintainer.")
Copy link

@fingolfin fingolfin Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is weird, but not actually a performance problem because the Julia compiler will never insert invocations to a three-argument Base.convert method


# Subtraits

"""
Expand Down
4 changes: 2 additions & 2 deletions src/interface.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,10 +606,10 @@ coordinates(obj) = coordinates(trait(obj), obj)
"""
convert(type::CustomGeom, geom)
Convert `geom` into the `CustomGeom` type if both geom as the CustomGeom package
have implemented GeoInterface.
Create a `CustomGeom` from any `geom` that implements the GeoInterface.
"""
convert(T, geom) = convert(T, geomtrait(geom), geom)
Copy link

@fingolfin fingolfin Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was changed to Base.convert, it would be highly problematic -- but not just because of potential ambiguities this introduces, but also because it would invalidate basically every method in the system, forcing Julia to recompile tons of stuff. So basically after using GeoInterface everything would have the "first time invocation overhead" again. That'd be really bad, esp. now that Julia 1.9 or 1.10 will add native code caching (i.e. generated machine code will be cached as part of precompilation -- speeding up TTFX quite a bit -- unless packages kill the benefit by invalidating methods...).

Copy link
Member

@rafaqz rafaqz Dec 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noone suggested that you may misunderstand this PR.

If we invalidate anything with convert it would be with type pyracy, which is much higher up the list of "things not to do".

But we are all on board with using the package convert it's just what to do with the fact that we already have implementations of Base.convert in the wild.

convert(::Type{T}, x::T) where {T} = x # no-op

"""
astext(geom) -> WKT
Expand Down
12 changes: 2 additions & 10 deletions test/test_primitives.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ using Test
GeoInterface.geomtrait(::MyCurve) = LineStringTrait()
GeoInterface.ngeom(::LineStringTrait, geom::MyCurve) = 2
GeoInterface.getgeom(::LineStringTrait, geom::MyCurve, i) = MyPoint()
Base.convert(T::Type{MyCurve}, geom::X) where {X} = Base.convert(T, geomtrait(geom), geom)
Base.convert(::Type{MyCurve}, ::LineStringTrait, geom::MyCurve) = geom

GeoInterface.isgeometry(::MyPolygon) = true
GeoInterface.geomtrait(::MyPolygon) = PolygonTrait()
Expand Down Expand Up @@ -235,15 +233,9 @@ end
struct XCurve end
struct XPolygon end

Base.convert(T::Type{XCurve}, geom::X) where {X} = Base.convert(T, geomtrait(geom), geom)
Base.convert(::Type{XCurve}, ::LineStringTrait, geom::XCurve) = geom # fast fallthrough
Base.convert(::Type{XCurve}, ::LineStringTrait, geom) = geom

geom = MyCurve()
@test !isnothing(convert(MyCurve, geom))

Base.convert(T::Type{XPolygon}, geom::X) where {X} = Base.convert(T, geomtrait(geom), geom)
@test_throws Exception convert(MyPolygon, geom)
@test GeoInterface.convert(MyCurve, geom) === geom
@test_throws Exception GeoInterface.convert(MyPolygon, geom)
end

@testset "Operations" begin
Expand Down