JuliaGeometry / CoordinateTransformations.jl

A fresh approach to coordinate transformations...
Other
180 stars 24 forks source link

Unitful compatibility #58

Closed SebastianM-C closed 4 years ago

SebastianM-C commented 4 years ago

The structures for the coordinate types, like Polar implicitly assume that the type for angles is the same as the type for the radius.

https://github.com/JuliaGeometry/CoordinateTransformations.jl/blob/383c023d90e71ee89e6f3728971a9fed3dcbc0a0/src/coordinatesystems.jl#L7-L10

If an additional type parameter would be added, I think it could make CoordinateTransformations.jl and Unitful.jl compatible,

struct Polar{T,A}
    r::T
    θ::A
end

If you are interested, I can try this in a PR.

andyferris commented 4 years ago

That's not a bad idea!

Be careful to consider cases like Polar(1.0, 1) which probably should use promote or some such (if both inputs are Number, say).

SebastianM-C commented 4 years ago

I'm not sure how to handle

Base.eltype(::Polar{T}) where {T} = T
Base.eltype(::Type{Polar{T}}) where {T} = T

when extending to two type parameters.

Should it use promote_type(T,A)?

SebastianM-C commented 4 years ago

I considered

Base.eltype(::Polar{T,A}) where {T,A} = promote_type(T, A)
Base.eltype(::Type{Polar{T,A}}) where {T,A} = promote_type(T, A)

in #60, but I'm not sure it makes sense.

c42f commented 4 years ago

I think eltype cannot make a lot of sense in the unitful case. Sure it can promote to Any in principle, but that's not useful for the way it seems to be used internally.

It seems a bit of a weird API to be extending anyway - maybe we should consider removing it?

andyferris commented 4 years ago

Agreed; it neither iterates nor supports in therefore eltype is a pun and should be removed.