Unitful compatibility

[SebastianM-C]

• Add separate type parameter for the angle
• Promote types for combinations of AbstractFloat and Integer

I did not consider <:Number because I didn't know how to do it without getting a stack overflow and also because Unitful subtypes Number and promoting to a common type is exactly the opposite of what this PR is trying to do.

I tried to cover the most common cases with the tests. Let me know what you think of this.
This closes #58

[andyferris]

Let's just drop eltype from Polar and friends. eltype is part of the iteration API and unlike Cartesian coordinates (in AbstractVectors) you can't iterate them.

There might be one or two places where you need some type (for zero(T)) but looking at that, that logic might need cleaning up in any case (these days I tend to use zero(x) where x is a value).

[andyferris]

The Inetger vs AbstractFloat thing will be very helpful. I supposing there will be corner cases with Dual and so-on, but if Unitful is using Number (which I take as at least being a semi-ring!) then there's not much we can do...

[SebastianM-C]

There might be one or two places where you need some type (for zero(T)) but looking at that, that logic might need cleaning up in any case (these days I tend to use zero(x) where x is a value).

Do you mean to say that I should add zero for Polar and the others?

Regarding the Inetger with AbstractFloat combinations, the old implementation had some restrictions for things like

julia> x=SVector{3}(1,0,0);

julia> CylindricalFromCartesian()(x)
ERROR: MethodError: no method matching Cylindrical(::Float64, ::Float64, ::Int64)
Closest candidates are:
  Cylindrical(::T, ::T, ::T) where T at C:\Users\sebastian\.julia\packages\CoordinateTransformations\HeCSs\src\coordinatesystems.jl:84
Stacktrace:
 [1] (::CylindricalFromCartesian)(::SArray{Tuple{3},Int64,1,3}) at C:\Users\sebastian\.julia\packages\CoordinateTransformations\HeCSs\src\coordinatesystems.jl:153
 [2] top-level scope at none:0

with this PR

julia> x=SVector{3}(1,0,0);

julia> CylindricalFromCartesian()(x)
Cylindrical(r=1.0, θ=0.0 rad, z=0.0)

Regarding the conversions, there is a slight discrepancy between the unitless and unitful cases for things like Polar(1.0,2) vs Polar(1.0u"m",2).

julia> Polar(1.0,2)
Polar(r=1.0, θ=2.0 rad)

julia> Polar(1.0u"m",2)
Polar(r=1.0 m, θ=2 rad)

This is because the number type is wrapped by Quantity (more about this at https://painterqubits.github.io/Unitful.jl/stable/types/) and I was not sure how to implement this.
There is something that addresses this in RecursiveArrayTools.jl, more precisely recursive_unitless_eltype which uses one in the Unitful case

https://github.com/SciML/RecursiveArrayTools.jl/blob/84f3051388de0c0ea984f34ff94a1db454bf46a8/src/utils.jl#L94

I was thinking that I could do something similar, but first I wanted to ask whether the logic for changing the type should be in the constructor or in the CartesianFromX transformations.
I also wanted to ask why is important to have a uniform type inside Polar (and friends).

Also I'm not sure what happens when you combine Dual numbers and Unitful or other types, but if you have ideas for some tests I can include them.

[andyferris]

The constructor is the right place to deal with promotion.

Note that these packages all define promote fine.

julia> using Unitful, DualNumbers

julia> x = 1u"m"
1 m

julia> y = 2.0
2.0

julia> d = dual(0x01, 0x02)
1 + 2ɛ

julia> promote(x, y)
(1.0 m, 2.0)

julia> promote(x, d)
(1 + 0ɛ m, 1 + 2ɛ)

julia> promote(y, d)
(2.0 + 0.0ɛ, 1.0 + 2.0ɛ)

For Polar I would do this non-recursive outer constructor:

function Polar(r, theta)
    (r2, theta2) = promote(r, theta)
    return Polar{typeof(r2), typeof(theta)}(r2, theta)
end

and let people use the inner constructor to override.

[SebastianM-C]

Thanks for the suggestion, this is much better than my previous implementation.
I implemented promotion as you said, but there is a small problem with testing.
It looks like isapprox is broken in the case of dimensionless numbers (<: DimensionlessQuantity) due to a missing dispatch. Should I wait for the upstream fix or should I just test the individual components (and use ustrip) to avoid the problem?

[andyferris]

I think @test_broken and waiting for upstream is quite sensible.