Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

one(measurement) should return 1, not 1 ± 0 #134

Closed
stevengj opened this issue Feb 14, 2023 · 8 comments · Fixed by #135
Closed

one(measurement) should return 1, not 1 ± 0 #134

stevengj opened this issue Feb 14, 2023 · 8 comments · Fixed by #135

Comments

@stevengj
Copy link

I would suggest defining:

Base.one(::Type{Measurement{T}}) where T = one(T)

which still a correct answer since it is still a multiplicative identity for a Measurement (type promotion takes care of the rest). In general, one(x) need not return the same type as x in Julia — if the caller wants the same type, they should use oneunit(x).

one(x) is commonly used to "strip the units" from a quantity, and it would provide a nice generic way to extract the underlying scalar type from a measurement. For example, this code in QuadGK.jl should work as-is for Measurement if you define one, and you should no longer need to implement specialized QuadGK methods. (Hopefully you can eliminate your QuadGK dependency completely and it will just work.)

@giordano
Copy link
Member

Uhm, if I define

Base.one(::Type{Measurement{T}}) where T = one(T)
Base.one(::Type{Complex{Measurement{T}}}) where T = one(Complex{T})

then some operations are no longer inferred correctly:

julia> @code_warntype(complex(measurement(3)) ^ 2.5)
MethodInstance for ^(::Complex{Measurement{Float64}}, ::Float64)
  from ^(z::Complex{T}, p::S) where {T<:Real, S<:Real} @ Base complex.jl:873
Static Parameters
  T = Measurement{Float64}
  S = Float64
Arguments
  #self#::Core.Const(^)
  z::Complex{Measurement{Float64}}
  p::Float64
Locals
  P::Type{Measurement{Float64}}
Body::Union{Complex{Measurement{Float64}}, ComplexF64}
1 ─      (P = Base.promote_type($(Expr(:static_parameter, 1)), $(Expr(:static_parameter, 2))))
│   %2 = Core.apply_type(Base.Complex, P::Core.Const(Measurement{Float64}))::Core.Const(Complex{Measurement{Float64}})
│   %3 = (%2)(z)::Complex{Measurement{Float64}}
│   %4 = (P::Core.Const(Measurement{Float64}))(p)::Core.PartialStruct(Measurement{Float64}, Any[Float64, Core.Const(0.0), Core.Const(0x0000000000000000), Core.Const(Measurements.Derivatives{Float64}())])
│   %5 = (%3 ^ %4)::Union{Complex{Measurement{Float64}}, ComplexF64}
└──      return %5

julia> A = [(14 ± 0.1) (23 ± 0.2); (-12 ± 0.3) (29 ± 0.4)];

julia> using LinearAlgebra

julia> @code_warntype det(A)
MethodInstance for LinearAlgebra.det(::Matrix{Measurement{Float64}})
  from det(A::AbstractMatrix{T}) where T @ LinearAlgebra ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/LinearAlgebra/src/generic.jl:1617
Static Parameters
  T = Measurement{Float64}
Arguments
  #self#::Core.Const(LinearAlgebra.det)
  A::Matrix{Measurement{Float64}}
Locals
  S::Type{Measurement{Float64}}
Body::Union{Float64, Measurement{Float64}}
1 ─       Core.NewvarNode(:(S))
│   %2  = LinearAlgebra.istriu(A)::Bool
└──       goto #3 if not %2
2 ─       goto #4
3 ─ %5  = LinearAlgebra.istril(A)::Bool
└──       goto #5 if not %5
4 ┄ %7  = $(Expr(:static_parameter, 1))::Core.Const(Measurement{Float64})
│   %8  = LinearAlgebra.one($(Expr(:static_parameter, 1)))::Core.Const(1.0)
│   %9  = LinearAlgebra.zero($(Expr(:static_parameter, 1)))::Core.Const(0.0 ± 0.0)
│   %10 = (%8 * %9)::Core.Const(0.0 ± 0.0)
│   %11 = LinearAlgebra.zero($(Expr(:static_parameter, 1)))::Core.Const(0.0 ± 0.0)
│   %12 = (%10 + %11)::Core.PartialStruct(Measurement{Float64}, Any[Core.Const(0.0), Float64, Core.Const(0x0000000000000000), Measurements.Derivatives{Float64}])
│   %13 = LinearAlgebra.one($(Expr(:static_parameter, 1)))::Core.Const(1.0)
│   %14 = (%12 / %13)::Core.PartialStruct(Measurement{Float64}, Any[Core.Const(0.0), Float64, Core.Const(0x0000000000000000), Measurements.Derivatives{Float64}])
│   %15 = LinearAlgebra.typeof(%14)::Core.Const(Measurement{Float64})
│         (S = LinearAlgebra.promote_type(%7, %15))
│   %17 = S::Core.Const(Measurement{Float64})
│   %18 = LinearAlgebra.UpperTriangular(A)::UpperTriangular{Measurement{Float64}, Matrix{Measurement{Float64}}}
│   %19 = LinearAlgebra.det(%18)::Core.PartialStruct(Measurement{Float64}, Any[Float64, Float64, Core.Const(0x0000000000000000), Measurements.Derivatives{Float64}])
│   %20 = LinearAlgebra.convert(%17, %19)::Core.PartialStruct(Measurement{Float64}, Any[Float64, Float64, Core.Const(0x0000000000000000), Measurements.Derivatives{Float64}])
└──       return %20
5 ─ %22 = (:check,)::Core.Const((:check,))
│   %23 = Core.apply_type(Core.NamedTuple, %22)::Core.Const(NamedTuple{(:check,)})
│   %24 = Core.tuple(false)::Core.Const((false,))
│   %25 = (%23)(%24)::Core.Const((check = false,))
│   %26 = Core.kwcall(%25, LinearAlgebra.lu, A)::LU{Measurement{Float64}, Matrix{Measurement{Float64}}, Vector{Int64}}
│   %27 = LinearAlgebra.det(%26)::Union{Float64, Measurement{Float64}}
└──       return %27

which breaks tests at

@test @inferred(z ^ 2.5) @inferred(x ^ 2.5)
@test @inferred(z ^ 3) @inferred(x ^ 3)
@test @inferred(det(A)) 682 ± 9.650906693155829

@giordano
Copy link
Member

And the accuracy of a bunch of QuadGK tests gets worse:

QuadGK: Test Failed at /home/mose/.julia/dev/Measurements/test/runtests.jl:852
  Expression: ≈((QuadGK.quadgk(sin, -y, y))[1], cos(-y) - cos(y), atol = eps(Float64))
   Evaluated: -2.23e-16 ± 2.7e-17 ≈ 0.0 ± 0.0 (atol=2.220446049250313e-16)
Stacktrace:
 [1] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/Measurements/test/runtests.jl:852 [inlined]
 [3] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/Measurements/test/runtests.jl:851
QuadGK: Test Failed at /home/mose/.julia/dev/Measurements/test/runtests.jl:855
  Expression: ≈((QuadGK.quadgk((t->(cos(x - t);)), 0, 2pi))[1], measurement(0), atol = 7.0e-16)
   Evaluated: -1.063e-13 ± 2.9e-15 ≈ 0.0 ± 0.0 (atol=7.0e-16)
Stacktrace:
 [1] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/Measurements/test/runtests.jl:855 [inlined]
 [3] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/Measurements/test/runtests.jl:851
QuadGK: Test Failed at /home/mose/.julia/dev/Measurements/test/runtests.jl:869
  Expression: ≈((QuadGK.quadgk(sin, -y, y))[1], #= /home/mose/.julia/dev/Measurements/test/runtests.jl:870 =# @uncertain(((x->begin
                (QuadGK.quadgk(sin, -x, x))[1]
            end))(y)), atol = 1.0e-10)
   Evaluated: -2.23e-16 ± 2.7e-17 ≈ -2.2e-16 ± 4.6e-10 (atol=1.0e-10)
Stacktrace:
 [1] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:477 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/Measurements/test/runtests.jl:869 [inlined]
 [3] macro expansion
   @ ~/.julia/juliaup/julia-1.9.0-beta4+0.x64.linux.gnu/share/julia/stdlib/v1.9/Test/src/Test.jl:1496 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/Measurements/test/runtests.jl:851
Test Summary: | Pass  Fail  Total   Time
QuadGK        |   16     3     19  14.0s

although the prospect of deleting the src/quadgk.jl file is quite appealing.

@giordano
Copy link
Member

giordano commented Feb 14, 2023

I presume the problem with the complex power is the use of one at https://github.com/JuliaLang/julia/blob/afeda9f8cf0776461eaf565333e2159122d70bc4/base/complex.jl#L789?

Edit: no, or not only that: replacing one with oneunit on that line doesn't fix the inference failure.

@giordano
Copy link
Member

And the accuracy of a bunch of QuadGK tests gets worse:

Actually, that's independent from this change, accuracy got worse in v2.8.1 of QuadGK, it's restored if I downgraded it to v2.8.0.

@giordano
Copy link
Member

giordano commented Feb 15, 2023

In particular, with 2.8.0:

julia> QuadGK.quadgk(t -> cos(3 - t), 0, 2pi)
(6.106226635438361e-16, 0.0)

with 2.8.1:

julia> QuadGK.quadgk(t -> cos(3 - t), 0, 2pi)
(-1.0625509660110193e-13, 1.1749641421160047e-23)

This has nothing to do with Measurements.jl

giordano added a commit that referenced this issue Feb 15, 2023
@giordano
Copy link
Member

giordano commented Feb 15, 2023

@stevengj Additionally, v2.6.0 of QuadGK broke another test here:

# Issue https://github.com/JuliaPhysics/Measurements.jl/issues/75
f(x) = x^2
F(x) = x^3 / 3
a = (5 ± 0.1)u"m"
b = (10 ± 1)u"m"
@test (QuadGK.quadgk(f, a, b)[1]).val (F(b) - F(a)).val

julia> using Unitful, Measurements, QuadGK

julia> f(x) = x^2
f (generic function with 1 method)

julia> F(x) = x^3 / 3
F (generic function with 1 method)

julia> a = (5 ± 0.1)u"m"
5.0 ± 0.1 m

julia> b = (10 ± 1)u"m"
10.0 ± 1.0 m

julia> QuadGK.quadgk(f, a, b)
ERROR: MethodError: no method matching Float64(::Measurement{Float64})
Closest candidates are:
  (::Type{T})(::Real, ::RoundingMode) where T<:AbstractFloat at rounding.jl:200
  (::Type{T})(::T) where T<:Number at boot.jl:772
  (::Type{T})(::AbstractChar) where T<:Union{AbstractChar, Number} at char.jl:50
  ...
Stacktrace:
  [1] convert(#unused#::Type{Float64}, x::Measurement{Float64})
    @ Base ./number.jl:7
  [2] setindex!(A::Vector{Float64}, x::Measurement{Float64}, i1::Int64)
    @ Base ./array.jl:966
  [3] eignewt(b::Vector{Measurement{Float64}}, m::Int64, n::Int64)
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/gausskronrod.jl:43
  [4] kronrod(#unused#::Type{Measurement{Float64}}, n::Int64)
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/gausskronrod.jl:197
  [5] macro expansion
    @ ~/.julia/packages/QuadGK/gxzkm/src/gausskronrod.jl:259 [inlined]
  [6] _cachedrule(#unused#::Type{Measurement{Float64}}, n::Int64)
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/gausskronrod.jl:259
  [7] cachedrule
    @ ~/.julia/packages/QuadGK/gxzkm/src/gausskronrod.jl:264 [inlined]
  [8] do_quadgk(f::typeof(f), s::Tuple{Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}, Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}, n::Int64, atol::Nothing, rtol::Nothing, maxevals::Int64, nrm::typeof(LinearAlgebra.norm), segbuf::Nothing)
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/adapt.jl:7
  [9] #46
    @ ~/.julia/packages/QuadGK/gxzkm/src/adapt.jl:219 [inlined]
 [10] handle_infinities(workfunc::QuadGK.var"#46#47"{Nothing, Nothing, Int64, Int64, typeof(LinearAlgebra.norm), Nothing}, f::typeof(f), s::Tuple{Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}, Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}})
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/adapt.jl:118
 [11] quadgk(::Function, ::Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}, ::Vararg{Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}}; atol::Nothing, rtol::Nothing, maxevals::Int64, order::Int64, norm::Function, segbuf::Nothing)
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/adapt.jl:218
 [12] quadgk(::Function, ::Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}}, ::Quantity{Measurement{Float64}, 𝐋, Unitful.FreeUnits{(m,), 𝐋, nothing}})
    @ QuadGK ~/.julia/packages/QuadGK/BYxcx/src/adapt.jl:216
 [13] top-level scope
    @ REPL[36]:1

Although this would be fixed in #135 by defining the one method you suggested.

giordano added a commit that referenced this issue Feb 15, 2023
* Use more `@inferred` in tests

* Reduce required accuracy of some QuadGK integrals

This regressed in v2.8.1:
<#134 (comment)>.

* Skip test broken upstream by QuadGK

* [CI] Use newer versions of `actions/checkout` workflow

* Do not test `@inferred` on `logabsbeta`
@stevengj
Copy link
Author

stevengj commented Feb 16, 2023

QuadGK.quadgk(t -> cos(3 - t), 0, 2pi)

The basic problem with this test is that you are asking it to compute an integral that is zero to a relative tolerance of ≈ 1e-8 (the default), which is basically impossible. So, the convergence criterion in practice depends on arbitrary details of roundoff errors, so e.g. slightly changing the order of the integrand evaluation (as we did in JuliaMath/QuadGK.jl@962c801 for 2.8.1) causes it to stop at a very different point.

The solution, as explained in the QuadGK manual, is that whenever you have an integral that might be zero, you should pass in a nonzero atol. Then it works fine in both 2.8.1, though it still can't converge to better than machine precision:

julia> quadgk_count(t -> cos(3 - t), 0, 2pi, atol=1e-15)
(2.220446049250313e-16, 5.551115123125783e-16, 105)

and 2.8.0:

julia> quadgk_count(t -> cos(3 - t), 0, 2pi, atol=1e-15)
(3.3306690738754696e-16, 2.220446049250313e-16, 105)

though the answers are very slightly different due to changes in roundoff.

@stevengj
Copy link
Author

stevengj commented Feb 16, 2023

For powers, oneunit is not correct since if you have a unitful number x then x^0 should be unitless, i.e. the x^n function is inherently type-unstable for dimensionful numbers.

The alternative would be to define some other method to get the "scalar" type from a numeric type. Maybe call it scalarone(x) or something like that, then define a package ScalarOne.jl and try to get all of the packages (Unitful, ForwardDiff, etcetera) to adopt it. It could call scalarone(x::Number) = one(x) as a fallback, so it would be no worse than what we have now, I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants