JuliaLang
rad-deg conversions for more types
rad-deg conversion is basically a simple scaling, and removing the explicit restriction makes it work for all types defining *, / both in Base and in packages
I'm personally not a fan of loosening signatures beyond necessity. ::Number looks to me a reasonable signature for such functions, what kind of non-Number types could benefit from being able to run rad2deg/deg2rad?
Within Base, I often wish these conversions worked with ranges and missings.
They are semantically just multiplications by the scaling factor, and should behave accordingly. Currently, we have:
# turns a range into a vector
julia> (0:90) .|> deg2rad
91-element Vector{Float64}:
...
# doesn't work
julia> (0:90) |> deg2rad
ERROR: MethodError: no method matching deg2rad(::UnitRange{Int64})
# works and returns a range
julia> (0:90) * pi/180
0.0:0.017453292519943295:1.5707963267948966
and
julia> missing |> deg2rad
ERROR: MethodError: no method matching deg2rad(::Missing)
julia> missing * pi/180
missing
Note that other functions aren't restricted to specific types, where it makes sense: eg https://github.com/JuliaLang/julia/blob/8499445de68f44ab9f8f44b446405bb8a11fe72b/base/math.jl#L1301.
From triage: this is potentially confusing since rad2deg(vec) would work whereas most similar cases require a dot. We do think the signature can be expanded to Union{Number, Missing} at least. Personally I don't mind dropping the type so much but it bothers some people.
For me, converting between degree and radian ranges is also a very important usecase, that won't be solved by adding Missing.
I find the triage decision unfortunate, this means the best way to convert rad/deg still remains x * pi/180 instead of deg2rad(x). The conversion is basically a multiplication, so I would personally expect it to behave accordingly.
One interesting problem with allowing this to be called on e.g. arrays is that it might give a different answer than broadcasting: we will compute array/pi first, followed by *180, instead of calling rad2deg on each element, which might use the float method (a single multiply) or indeed different custom implementations for each element. One example is Unitful, which changes between radian and degree units in these functions.
A possible improvement is to define new Irrationals for pi/180 and 180/pi, so we can define these functions directly as scaling by those constants, but that still wouldn't handle the Unitful case.
Great point!
While conceptually deg <-> rad is just a multiplication, it's not always the case in code.
Still, it's a linear operation, and linear operations tend to work directly on arrays in Julia: examples are addition and multiplication by a number (such as pi/180 :) ).
Such arithmetic operations often have separate implementations for arrays and ranges. Adding a few simple deg2rad methods in this fashion would solve all issues:
- Ranges can be converted, keeping them ranges and not arrays
- Everything works correctly, even with fancy types like Unitful (and Unitful ranges!)
Implementation would just broadcast deg2rad for abstractarrays, with separate methods for ranges - exactly as in https://github.com/JuliaLang/julia/blob/24f53313eed272f6d783c2e241fcd72dc342a2df/base/range.jl#L1258-L1266 with deg2rad instead of -. Even the comment there fits, "linear operations on ranges".
Thanks for the comments above I realized that removing the type constraint is wrong and can cause incorrect behavior.
However, extending deg <-> rad conversions to arrays is totally fine, and nicely fits other linear operations implemented for them. Stuff like real(A), -A, etc works for all arrays automatically in Julia, and correctly returns ranges when passed a range. Here, I just add deg2rad and rad2deg to this list of operations.
Also, adding Missing following @JeffBezanson's comment.
Will add tests if this change is OK as it is.
Pardon my curiosity but why do you prefer deg2rad(0:90) over deg2rad.(0:90)? They have the same return type, are equally performant, and take pretty much the same effort to type. What am I missing?
Well, that's not the case :)
They have the same return type
julia> deg2rad.(0:90)
91-element Vector{Float64}:
julia> deg2rad(0:90) # this PR
91-element LinRange{Float64, Int64}:
are equally performant
julia> @btime deg2rad.(0:90);
103.393 ns (1 allocation: 816 bytes)
julia> @btime deg2rad(0:90); # this PR
1.267 ns (0 allocations: 0 bytes)
I see! I didn't notice the methods for ranges before. Does the current implementation give exact results though (is collect(rad2deg(x)) == rad2deg.(x))?
Thanks for pointing this out! Now this equality seems to hold, I followed the approach of how it's done elsewhere in the codebase.
Don't understand why CI fails, any pointers?..
I believe for f in (rad2deg, deg2rad) should iterate over symbols: for f in (:rad2deg, :deg2rad). Although I am not sure if that is what caused the build failure.
However, I still have doubts about the numerical accuracy. For example:
julia> collect(rad2deg(2:10)) == rad2deg.(2:10)
true
# but
julia> collect(rad2deg(1:10)) == rad2deg.(1:10)
false
The broadcasting approach simply applies function f to every element of range r, resulting in an array a, hence a[i] = f(r[i]). On the contrary, as I understand it, your approach boils down to a[i] = f(first(r)) + (i - 1) * s, where s = f(step(r)) or s = (f(last(r)) - f(first(r))) / length(r) depending on the type of r. So despite being theoretically correct for a linear f, it seems to me that it will always be less numerically accurate than the broadcasting approach. I might be wrong though.
(Have you considered JuliaArrays/MappedArrays.jl for your use-case? It supports ranges as well.)
I think this kind of accuracy is too high of a bar to require from a function. It doesn't even hold for simple multiplication:
julia> f(x) = x*1.2345
julia> collect(f(1:10)) == f.(1:10)
false
Floating point computations are approximate anyway.
(Have you considered JuliaArrays/MappedArrays.jl for your use-case? It supports ranges as well.)
Yes, there are lazy map implementations like this (though I prefer FlexiMaps.mapview for generality :) ), but the result isn't a range:
julia> r = mappedarray(deg2rad, 1:10)
10-element mappedarray(deg2rad, ::UnitRange{Int64}) with eltype Float64:
...
julia> step(r)
ERROR: MethodError: no method matching step
UPD: FlexiMaps.mapview now does preserve a range when possible:
julia> using FlexiMaps
julia> r = mapview(deg2rad, 1:10)
0.017453292519943295:0.017453292519943295:0.17453292519943295
julia> step(r)
0.017453292519943295
Bump @giordano @JeffBezanson (pinging those who commented earlier in the discussion)
if you remove the ::AbstractArray methods I suspect you will find more success with the remaining components of the PR. I still feel like those are unnecessary to add
I do not think that expanding of the scope of rad2deg and deg2rad to arrays is a good idea. The reason multiplication and division support array/scalar interactions is because of the mathematical notion of vector-scalar products. The only justification I can see here is for performance, so instead I recommend specializing on broadcasted ranges.
Widening the type signature to accept missing is okay. I'm not enthusiastic about it, but it does seem appropriate.
(aside: I don't like the functions at all #54144)
