Why dispatch on concrete types?

[rafaqz]

I'm wondering why Vector is used everywhere for dispatch, instead of AbstractVector? and Float64 instead of Number?

[visr]

I don't think there are hard reasons for this. It probably was easiest to start off with concrete types to hash out a first design, but it would be good to work towards making this more generic.

[yeesian]

#20 (comment) was what I was thinking when I first wrote this package. That thread is also the latest discussion on it that I know of. I'm currently bound up in graduation plans, but PRs are definitely welcome!

[rafaqz]

Ok that makes sense. I'll look at doing a refactor.

It also seems we could cut half the code in this package without losing functionality. It would be more inviting as an api if it was simpler.

[rafaqz]

I've just done most of a massive refactor. But I just realised there aren't enough tests to verify a pull request, and I've definately made a mistake or two.

Would running the tests for Shapefile.jl, ArchGDAL and GeoJSON be adequate?

[visr]

Yeah testing those packages would be good, and I'd also add LibGEOS.

[rafaqz]

Some comments on generalising GeoInterface:

Position not being <: AbstractPosition (and just being Vector{Float64}) is a barrier to abstraction, as we can't add methods to the constructor for Position to handle bad inputs

So letting MultiPolygon and similar hold other kinds of positions (say StaticArrays) isn't so easy. It works, but generalising everything loses feature you're currently getting for free like converting from vectors of Any (returned by JSON.jl) to vectors of Float64 with Position[].

It's also it harder to simplify dispatch because we always need to use Union{Position, AbstractPosition} which leads to ambiguities, that Position <: AbstractPosition would resolve.

[visr]

Yeah I can understand that it's hard to generalize this completely without breaking something. That's why it is good to discuss and try out approaches. I think we all agree that const Position = Vector{Float64} simple as it is, does not have the future :). We also seem to agree that Vector{Float64} should definitely be accepted as a position, just like SVector{3, Float32}.

I would say that the primary aim of this package is to define an interface that various Julia types as well as pointers to C libraries can subscribe to. There is a clear need for this since different packages have different needs, so we cannot just all agree on one type to cover all use cases. We should however strive for an interface generic enough to cover most if not all use cases.

Secondly we currently also currently provide concrete types, which allow us to create some sort of 'canonical' julia type representation out of whatever GeoInterface-complying type you throw in. This type is currently based on GeoJSON, since that was also the starting point of Python's __geo_interface__. Ideally you can use any GeoInterface-complying point everywhere, such that you'd never have to convert to this package's types. That leads me to think, should there even be a concrete, 'canonical' implementation in this package? The GeoJSON tyhpe

We can learn from the success of Tables.jl in this regard. If there is one most standard table type, it's probably DataFrame. But it can happily live in another package. Besides Tables.jl it's also worth studying RoamesGeometry.jl. Perhaps we should even do away with the point/position distinction, and just have points be AbstractVector{<:Real}, and use a length trait for 2 vs 3D. In general a traits based geo interface seems like a better fit than relying on subtyping, given the diversity of the potential subscribers.

[rafaqz]

Removing AbstractPoint and making point <: AbstractVector{<:Number} would also simplify things. (<:Number to allows Unitful units and other custom types that don't fit in Real should anyone ever need them)

Then we just need to add tests for SVector etc and make sure any arbitrary vector types are allowed as points.

I also agree on moving some responsibility to GeoJSON. Specifically converting Vector{Any} to Vector{Float64}. Unless GeoInterface has a canonical type it shouldn't know what to convert it to. It should just allow any points in AbstractVector{<:Number}, and otherwise give a MethodError. Otherwise it could try to convert AbstractVector{Any} to AbstractVector{Float64} and allow AbstractVector{Any} as an input type everywhere, but it will be messy.

Maybe using a different JSON package like JSON2 will give Vector{Float64} instead of Vector{Any}? then we wont need a conversion step at all.

[evetion]

Closed by #33.