From 3c5ec20f04097e2394c1f7585846d1e89075c11a Mon Sep 17 00:00:00 2001
From: Sebastian Stock <42280794+sostock@users.noreply.github.com>
Date: Mon, 30 Dec 2024 23:11:44 +0100
Subject: [PATCH] Fix conversion to unitless `Quantity` (#724)

---
 src/conversion.jl | 10 ++++------
 test/runtests.jl  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/src/conversion.jl b/src/conversion.jl
index e244911a..25b99536 100644
--- a/src/conversion.jl
+++ b/src/conversion.jl
@@ -115,17 +115,15 @@ uconvert(a::Units, x::Missing) = missing
     t1 = a <: AffineUnits ? a.parameters[end].parameters[end] :
         :(zero($(x.parameters[1])))
     quote
-        dimension(a) != dimension(x) && return throw(DimensionError(a, x))
+        dimension(a) != dimension(x) && throw(DimensionError(a, x))
         return Quantity(((x.val - $t0) * $conv) + $t1, a)
     end
 end
 
 function convert(::Type{Quantity{T,D,U}}, x::Number) where {T,D,U}
-    if dimension(x) == D
-        Quantity(T(uconvert(U(),x).val), U())
-    else
-        throw(DimensionError(U(),x))
-    end
+    (dimension(x) != D) && throw(DimensionError(U(), x))
+    q = uconvert(U(), x)
+    Quantity{T,D,U}(isa(q, AbstractQuantity) ? q.val : q)
 end
 
 # needed ever since julialang/julia#28216
diff --git a/test/runtests.jl b/test/runtests.jl
index 8b905fc9..adb64490 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -144,6 +144,14 @@ end
         @test convert(Int, 1*FreeUnits(m/mm)) === 1000
         @test convert(Int, 1*FixedUnits(m/mm)) === 1000
         @test convert(Int, 1*ContextUnits(m/mm, NoUnits)) === 1000
+        for U = (NoUnits, FixedUnits(NoUnits), ContextUnits(NoUnits, m/mm))
+            @test convert(Quantity{Int,NoDims,typeof(U)}, 1*FreeUnits(m/mm)) === Quantity{Int,NoDims,typeof(U)}(1000)
+            @test convert(Quantity{Int,NoDims,typeof(U)}, 1*FixedUnits(m/mm)) === Quantity{Int,NoDims,typeof(U)}(1000)
+            @test convert(Quantity{Int,NoDims,typeof(U)}, 1*ContextUnits(m/mm, NoUnits)) === Quantity{Int,NoDims,typeof(U)}(1000)
+            @test convert(Quantity{Int,NoDims,typeof(U)}, 1) === Quantity{Int,NoDims,typeof(U)}(1)
+        end
+        @test convert(Quantity{Int}, 1) === Quantity{Int,NoDims,typeof(NoUnits)}(1)
+        @test convert(Quantity{Int,NoDims}, 1) === Quantity{Int,NoDims,typeof(NoUnits)}(1)
 
         # w/ units distinct from w/o units
         @test 1m != 1
@@ -1856,6 +1864,42 @@ end
         @test convert(Float64, u"10dB_p") === 10.0
         @test convert(Float64, u"20dB_rp") === 10.0
 
+        @test_throws ErrorException convert(typeof(1.0dB/s), 5.0Hz)
+        @test convert(typeof(1.0dB_p/s), 100.0Hz) === 20.0dB_p/s
+        @test convert(typeof(1.0dB_rp/s), 100.0Hz) === 40.0dB_rp/s
+
+        @test_throws ErrorException convert(typeof(1.0dB/rad), 5.0)
+        @test convert(typeof(1.0dB_p/rad), 100.0) === 20.0dB_p/rad
+        @test convert(typeof(1.0dB_rp/rad), 100.0) === 40.0dB_rp/rad
+
+        @test convert(typeof(1.0m/cm), 40.0dB_rp) === 1.0m/cm
+        @test convert(typeof(1.0dB_p/s), 1.0dB_p/s) === 1.0dB_p/s
+
+        # This currently (and unnecessarily) involves a conversion to linear and back to logarithmic.
+        # This is lossy due to floating-point, therefore broken.
+        @test_broken convert(Quantity{typeof(1.0dB_rp), NoDims, typeof(Unitful.NoUnits)}, 1dB_rp) === 1.0dB_rp
+        @test_broken convert(typeof(1.0dB_p/s), 1dB_p/s) === 1.0dB_p/s # conversion to linear and back to logarithmic → lossy due to floating-point
+        @test isapprox(convert(typeof(1.0dB_p/s), 1dB_p/s), 1.0dB_p/s, rtol = 1e-3, atol=0/s)
+
+        # Wrongly throwing DimensionError
+        @test_broken convert(typeof(1.0dBm/s), 5.0mW*Hz) === @dB(5.0mW/1mW)/s
+        @test convert(typeof(1.0u"dBFS/rad"), 100.0) === @dB(100.0/1, true)/rad
+        @test_broken convert(typeof(1.0dBm/rad), 20.0dBm) === @dB(100.0mW/1mW)/rad
+        @test convert(typeof(1.0u"dBFS/rad"), 5.0u"dBFS") === 5.0u"dBFS/rad"
+        @test_broken convert(Quantity{typeof(1.0dBm), dimension(Unitful.mW), typeof(Unitful.NoUnits)}, 5.0dBm) === Quantity{typeof(1.0dBm), dimension(Unitful.mW), typeof(Unitful.NoUnits)}(5.0dBm)
+        @test convert(typeof(1.0m/cm), 40.0u"dBFS") === 1.0m/cm
+
+        for L = (40u"dB_rp", 20u"dB_p", 40u"dBFS")
+            for U = (NoUnits, FixedUnits(NoUnits), ContextUnits(NoUnits, m/mm))
+                @test convert(Quantity{Int,NoDims,typeof(U)}, L) === Quantity{Int,NoDims,typeof(U)}(100)
+                @test convert(Quantity{Int,NoDims,typeof(U)}, L/rad) === Quantity{Int,NoDims,typeof(U)}(100)
+            end
+            @test convert(Quantity{Int}, L) === Quantity{Int,NoDims,typeof(NoUnits)}(100)
+            @test convert(Quantity{Int}, L/rad) === 100/rad
+            @test convert(Quantity{Int,NoDims}, L) === Quantity{Int,NoDims,typeof(NoUnits)}(100)
+            @test convert(Quantity{Int,NoDims}, L/rad) === 100/rad
+        end
+
         @test isapprox(uconvertrp(NoUnits, 6.02dB), 2.0, atol=0.001)
         @test uconvertrp(NoUnits, 1Np) ≈ MathConstants.e
         @test uconvertrp(Np, MathConstants.e) == 1Np