refactor!: Consistent conversion rules and a frame-tag design for CartesianCoords

src/cartesian.jl

@@ -1,23 +1,58 @@
 """
-    CartesianCoords{TC <: AbstractSkyCoords, TF} <: AbstractSkyCoords
+    CartesianCoords{TC <: AbstractSkyCoords, TF <: Real} <: AbstractSkyCoords
     CartesianCoords{TC}(x, y, z)
+    CartesianCoords{TC, TF}(x, y, z)
+    CartesianCoords{TC}(vec::AbstractVector)
     CartesianCoords(c::AbstractSkyCoords)
 
-Cartesian representation of arbitrary coordinate systems.
-The type paramter `TC` identifies the coordinate scheme being encoded.
-Instances of this type should be created using the [`cartesian`](@ref) function.
+Cartesian representation of a sky coordinate as a 3-vector on the unit sphere.
 
-Note: since all sky coordinates are angle measures, the Cartesian coordinates
-are assumed to lie on the unit sphere.
+The type parameter `TC` is a *frame tag*: the element-type-free constructor of
+the coordinate system being represented, e.g., [`ICRSCoords`](@ref),
+[`GalCoords`](@ref), or [`FK5Coords{2000}`](@ref).
+
+Instances are typically created with [`cartesian`](@ref). The element type of
+the stored vector is carried by `TF` alone, so `cartesian(ICRSCoords{Float32}(0.1, 0.2))`,
+would return a `CartesianCoords{ICRSCoords, Float32}`.
+
+A fully parameterized tag such as `CartesianCoords{ICRSCoords{Float16}}` is also accepted.
+Its element type then determines the element type of the stored vector,
+so `CartesianCoords{ICRSCoords{Float16}}(c)` holds `Float16` data.
+Combining a parameterized tag with a conflicting explicit `TF` throws an `ArgumentError`,
+(e.g., `CartesianCoords{ICRSCoords{Float16}, Float64}(c)`) so the frame tag and the data
+can never disagree.
 """
 struct CartesianCoords{TC <: AbstractSkyCoords, TF <: Real} <: AbstractSkyCoords
     vec::SVector{3, TF}
+
+    function CartesianCoords{TC, TF}(vec) where {TC <: AbstractSkyCoords, TF <: Real}
+        TCF = _eltype(TC)
+        if !(TCF === TF || TCF === nothing)
+            throw(ArgumentError("Element type $TF conflicts with the frame tag $TC. Use the element-type-free tag $(constructorof(TC)) or the matching element type $TCF"))
+        end
+        return new{TC, TF}(vec)
+    end
 end
 
-CartesianCoords{TC}(args::Real...) where {TC} = CartesianCoords{TC}(SVector(float.(args)...))
-CartesianCoords{TC}(vec::AbstractVector{TF}) where {TC, TF} = CartesianCoords{TC, TF}(vec)
+# The element type implied by a coordinate type:
+#   - `Float32` for `ICRSCoords{Float32}`
+#   - `nothing` for an element-type-free frame tag such as
+#     `ICRSCoords` or `FK5Coords{2000}`
+_eltype(::Type{TC}) where {TC <: AbstractSkyCoords} = isconcretetype(TC) ? fieldtype(TC, 1) : nothing
+_eltype(::Type{CartesianCoords{TC, TF}}) where {TC <: AbstractSkyCoords, TF <: Real} = TF
+
+# Construction from raw components. As for the spherical types, an unspecified
+# element type is chosen from the frame tag if it carries one, otherwise from
+# the input (and floated); an explicit `TF` is honored exactly.
+CartesianCoords{TC}(args::Real...) where {TC <: AbstractSkyCoords} = CartesianCoords{TC}(SVector(float.(args)...))
+CartesianCoords{TC}(vec::AbstractVector{TF}) where {TC <: AbstractSkyCoords, TF <: Real} =
+    CartesianCoords{TC, something(_eltype(TC), float(TF))}(vec)
+CartesianCoords{TC, TF}(args::Real...) where {TC <: AbstractSkyCoords, TF <: Real} = CartesianCoords{TC, TF}(SVector{3, TF}(args))
+
+# Construction from another coordinate: delegate to `convert`,
+# which treats unspecified type parameters as "infer from the input"
 CartesianCoords(c::AbstractSkyCoords) = convert(CartesianCoords, c)
-CartesianCoords{TC}(c::AbstractSkyCoords) where {TC} = convert(CartesianCoords{TC}, c)
+CartesianCoords{TC}(c::AbstractSkyCoords) where {TC <: AbstractSkyCoords} = convert(CartesianCoords{TC}, c)
 CartesianCoords{TC, TF}(c::AbstractSkyCoords) where {TC <: AbstractSkyCoords, TF <: Real} = convert(CartesianCoords{TC, TF}, c)
 constructorof(::Type{<:CartesianCoords{TC}}) where {TC} = CartesianCoords{TC}
 
@@ -30,32 +65,54 @@ coords2cart(c::CartesianCoords) = vec(c)
 
 Returns a Cartesian representation of the coordinate `c` projected onto the unit sphere as a [`CartesianCoords`](@ref).
 
+The result is tagged with the frame of `c` and the numeric type of the data `c` contains; e.g.
+`cartesian(ICRSCoords(0.1, 0.2)) isa CartesianCoords{ICRSCoords, Float64}`.
+
 ### See also
 [`spherical`](@ref)
 """
 cartesian(c::CartesianCoords) = c
-cartesian(c::T) where {T <: AbstractSkyCoords} = CartesianCoords{T}(coords2cart(lon(c), lat(c)))
+cartesian(c::AbstractSkyCoords) = CartesianCoords{constructorof(typeof(c))}(coords2cart(c))
 
 """
     spherical(c::AbstractSkyCoords)
 
-Returns the spherical representation of the coordinate `c`. Coordinate arguments that are already in spherical coordinates are simply returned. If `c` is a [`CartesianCoords`](@ref) object, the Cartesian representation is converted to spherical and returned as a `T <: AbstractSkyCoords` where `c::CartesianCoords{T}`. 
+Returns the spherical representation of the coordinate `c`. Coordinate arguments that are already in spherical coordinates are simply returned. If `c` is a [`CartesianCoords`](@ref) object, the Cartesian representation is converted to spherical and returned as `TC(lon, lat)` where `c::CartesianCoords{TC}`.
 
 ### See also
 [`cartesian`](@ref)
 """
 spherical(c::AbstractSkyCoords) = c
-spherical(c::CartesianCoords{T}) where {T <: AbstractSkyCoords} = T(cart2coords(vec(c))...)
+spherical(c::CartesianCoords{TC}) where {TC <: AbstractSkyCoords} = TC(cart2coords(vec(c))...)
+
+# `convert` handles target type parameters uniformly: parameters that are
+# specified are honored exactly (the `convert` contract requires returning the
+# requested type); unspecified ones are inferred from the input. Every method
+# reduces to the two orthogonal primitives: representation change
+# (`cartesian`/`spherical`) and frame rotation (`rotmat`).
+
+# No parameters: keep the input frame, change representation only.
+Base.convert(::Type{CartesianCoords}, c::AbstractSkyCoords) = cartesian(c)
+Base.convert(::Type{CartesianCoords}, c::CartesianCoords) = c
 
-Base.convert(::Type{T}, c::CartesianCoords{S}) where {T <: AbstractSkyCoords, S <: AbstractSkyCoords} =
-    spherical(convert(CartesianCoords{T}, c))
-Base.convert(::Type{<:CartesianCoords{T}}, c::S) where {T <: AbstractSkyCoords, S <: AbstractSkyCoords} =
-    convert(CartesianCoords{T}, cartesian(c))
-Base.convert(::Type{<:CartesianCoords{T}}, c::CartesianCoords{S}) where {T <: AbstractSkyCoords, S <: AbstractSkyCoords} =
-    CartesianCoords{T}(rotmat(T, S) * vec(c))
+# Frame tag specified: rotate. Element type inferred from the rotated vector.
+Base.convert(::Type{CartesianCoords{TC}}, c::AbstractSkyCoords) where {TC <: AbstractSkyCoords} = convert(CartesianCoords{TC}, cartesian(c))
+Base.convert(::Type{CartesianCoords{TC}}, c::CartesianCoords{SC}) where {TC <: AbstractSkyCoords, SC <: AbstractSkyCoords} = CartesianCoords{TC}(rotmat(TC, SC) * vec(c))
+Base.convert(::Type{CartesianCoords{TC}}, c::CartesianCoords{TC}) where {TC <: AbstractSkyCoords} = c
 
+# Frame tag and element type specified: both honored exactly.
+Base.convert(::Type{CartesianCoords{TC, TF}}, c::AbstractSkyCoords) where {TC <: AbstractSkyCoords, TF <: Real} = convert(CartesianCoords{TC, TF}, cartesian(c))
+Base.convert(::Type{CartesianCoords{TC, TF}}, c::CartesianCoords{SC}) where {TC <: AbstractSkyCoords, TF <: Real, SC <: AbstractSkyCoords} = CartesianCoords{TC, TF}(rotmat(TC, SC) * vec(c))
+Base.convert(::Type{CartesianCoords{TC, TF}}, c::CartesianCoords{TC, TF}) where {TC <: AbstractSkyCoords, TF <: Real} = c
+
+# Spherical target from a Cartesian source: rotate, then change representation.
+function Base.convert(::Type{T}, c::CartesianCoords{SC}) where {T <: AbstractSkyCoords, SC <: AbstractSkyCoords}
+    r = rotmat(T, SC) * vec(c)
+    return T(cart2coords(r)...)
+end
 
-Base.:(==)(a::T, b::T) where {T <: CartesianCoords} = vec(a) == vec(b)
+Base.:(==)(a::CartesianCoords, b::CartesianCoords) = constructorof(typeof(a)) == constructorof(typeof(b)) && vec(a) == vec(b)
+Base.hash(c::CartesianCoords, h::UInt) = hash(vec(c), hash(constructorof(typeof(c)), h))
 Base.isapprox(a::CartesianCoords{TC}, b::CartesianCoords{TC}; kwargs...) where {TC} =
     isapprox(vec(a), vec(b); kwargs...)
 Base.isapprox(a::CartesianCoords{TCa}, b::CartesianCoords{TCb}; kwargs...) where {TCa, TCb} =

src/types.jl

@@ -136,7 +136,13 @@ function Base.convert(::Type{T}, c::S) where {T <: AbstractSkyCoords, S <: Abstr
     return T(lon, lat)
 end
 
-Base.:(==)(a::T, b::T) where {T <: AbstractSkyCoords} = lon(a) == lon(b) && lat(a) == lat(b)
+# Two coordinates are equal when they are in the same frame, identified by the
+# element-type-free frame tag, `constructorof`, and their angles are equal.
+# Element types need not match, just like `1.0 == 1.0f0`. `hash` mirrors this
+# so that value-equal coordinates of different element types collide in
+# `Dict`/`Set` as required by the `hash` contract.
+Base.:(==)(a::AbstractSkyCoords, b::AbstractSkyCoords) = constructorof(typeof(a)) == constructorof(typeof(b)) && lonlat(a) == lonlat(b)
+Base.hash(c::AbstractSkyCoords, h::UInt) = hash(lonlat(c), hash(constructorof(typeof(c)), h))
 Base.isapprox(a::ICRSCoords, b::ICRSCoords; kwargs...) = isapprox(SVector(lon(a), lat(a)), SVector(lon(b), lat(b)); kwargs...)
 Base.isapprox(a::GalCoords, b::GalCoords; kwargs...) = isapprox(SVector(lon(a), lat(a)), SVector(lon(b), lat(b)); kwargs...)
 Base.isapprox(a::SuperGalCoords, b::SuperGalCoords; kwargs...) = isapprox(SVector(lon(a), lat(a)), SVector(lon(b), lat(b)); kwargs...)

test/runtests.jl

@@ -163,6 +163,69 @@ end
     @test separation(a, b) ≈ separation(a3, b3) ≈ separation(a, b3) ≈ separation(a3, b)
 end
 
+@testset "CartesianCoords type parameters ($CT, $TF)" for TF in (Float32, Float64), CT in (ICRSCoords, GalCoords, FK5Coords{2000})
+    c = CT{TF}(0.1, 0.2)
+
+    # canonical form: element-type-free frame tag, element type carried by TF only
+    cart = @inferred cartesian(c)
+    @test cart isa CartesianCoords{CT, TF}
+
+    # round trip preserves the type exactly
+    rt = @inferred spherical(cart)
+    @test typeof(rt) === typeof(c)
+    @test rt ≈ c
+
+    # every spelling of "convert to Cartesian" agrees; unspecified parameters
+    # are inferred from the input
+    @test @inferred(CartesianCoords(c)) === cart
+    @test @inferred(CartesianCoords{CT}(c)) === cart
+    @test @inferred(CartesianCoords{CT, TF}(c)) === cart
+    @test convert(CartesianCoords, c) === cart
+    @test convert(CartesianCoords{CT}, c) === cart
+    @test convert(CartesianCoords{CT, TF}, c) === cart
+    @test (c |> CartesianCoords) === cart
+
+    # identity conversions short-circuit
+    @test cartesian(cart) === cart
+    @test CartesianCoords(cart) === cart
+    @test convert(CartesianCoords, cart) === cart
+    @test convert(CartesianCoords{CT}, cart) === cart
+    @test convert(CartesianCoords{CT, TF}, cart) === cart
+
+    # an explicitly requested element type is honored exactly (convert contract)
+    for TF2 in (Float32, Float64, BigFloat)
+        @test convert(CartesianCoords{GalCoords, TF2}, c) isa CartesianCoords{GalCoords, TF2}
+        @test CartesianCoords{GalCoords, TF2}(c) isa CartesianCoords{GalCoords, TF2}
+        @test convert(CartesianCoords{CT, TF2}, cart) isa CartesianCoords{CT, TF2}
+    end
+
+    # fully parameterized frame tags remain valid and are honored literally
+    cc = CartesianCoords{CT{TF}}(c)
+    @test cc isa CartesianCoords{CT{TF}, TF}
+    @test vec(cc) == vec(cart)
+    @test spherical(cc) isa CT{TF}
+    @test cc ≈ cart
+
+    # a parameterized tag determines the element type of the data, so the tag
+    # and the stored vector can never disagree
+    TF2 = TF === Float32 ? Float64 : Float32
+    cc2 = @inferred CartesianCoords{CT{TF2}}(c)
+    @test cc2 isa CartesianCoords{CT{TF2}, TF2}
+    @test convert(CartesianCoords{CT{TF2}}, c) isa CartesianCoords{CT{TF2}, TF2}
+    @test convert(CartesianCoords{CT{TF2}}, cart) isa CartesianCoords{CT{TF2}, TF2}
+    @test spherical(cc2) isa CT{TF2}
+    @test cc2 ≈ cart
+
+    # a conflicting explicit element type is an incoherent state and throws
+    @test_throws ArgumentError CartesianCoords{CT{TF2}, TF}(1, 0, 0)
+    @test_throws ArgumentError convert(CartesianCoords{CT{TF2}, TF}, c)
+
+    # conversion from Cartesian back to spherical honors requested parameters
+    @test convert(GalCoords{Float32}, cart) isa GalCoords{Float32}
+    @test convert(GalCoords, cart) isa GalCoords
+    @test GalCoords(cart) ≈ convert(GalCoords, c)
+end
+
 @testset "constructionbase" begin
     @test setproperties(ICRSCoords(1, 2), ra = 3) == ICRSCoords(3, 2)
     @test setproperties(GalCoords(1, 2), l = 3) == GalCoords(3, 2)
@@ -277,15 +340,33 @@ end
         c3 = T{Float32}(1.0, 2.0)
         c4 = T{Float32}(1.0, 2.001)
         @test c1 == c1
-        @test_broken c1 == c3
+        @test c1 == c3
+        @test c1 != c2
+        @test c1 != c4
         @test c1 ≈ c1
         @test c1 ≈ c3
         @test !(c1 ≈ c2)
         @test !(c1 ≈ c4)
         @test c1 ≈ c2  rtol = 1.0e-3
         @test c1 ≈ c4  rtol = 1.0e-3
+
+        # `==` implies equal hashes, so value-equal coordinates of different
+        # element types collapse in a Set; c2 and c4 stay distinct because
+        # 2.001 rounds to different values in Float32 and Float64
+        @test hash(c1) == hash(c3)
+        @test length(Set([c1, c2, c3, c4])) == 3
     end
 
+    # different frames never compare equal, even with equal angles
+    @test ICRSCoords(1, 2) != GalCoords(1, 2)
+    @test FK5Coords{2000}(1, 2) != FK5Coords{1950}(1, 2)
+    @test ICRSCoords(0, 0) != cartesian(ICRSCoords(0, 0))
+
+    # CartesianCoords: same frame tag and equal vectors, any element type
+    @test CartesianCoords{ICRSCoords}(1, 0, 0) == CartesianCoords{ICRSCoords, Float32}(1, 0, 0)
+    @test hash(CartesianCoords{ICRSCoords}(1, 0, 0)) == hash(CartesianCoords{ICRSCoords, Float32}(1, 0, 0))
+    @test CartesianCoords{ICRSCoords}(1, 0, 0) != CartesianCoords{GalCoords}(1, 0, 0)
+
     @test_broken (!(ICRSCoords(1, 2) ≈ FK5Coords{2000}(1, 2)); true)
     @test_broken (!(FK5Coords{2000}(1, 2) ≈ FK5Coords{1950}(1, 2)); true)
 end