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

Compatibility with QuadGK #597

Closed
aurelio-amerio opened this issue Dec 13, 2022 · 13 comments
Closed

Compatibility with QuadGK #597

aurelio-amerio opened this issue Dec 13, 2022 · 13 comments

Comments

@aurelio-amerio
Copy link

aurelio-amerio commented Dec 13, 2022

Hello, the update of QuadGK (a popular library for numerical integration) from v2.5 to v2.6 broke compatibility with Unitful.

To restore compatibility, one would have to extend the QuadGK function cachedrule to also work with quantities with something like

@generated function cachedrule(::Type{T}, n::Integer) where {T<:AbstractQuantity}
    TF = typeof(float(real(one(T))))
    cache = haskey(rulecache, TF) ? rulecache[TF] : (rulecache[TF] = Dict{Int,NTuple{3,Vector{TF}}}())
    :(haskey($cache, n) ? $cache[n] : ($cache[n] = kronrod($T, n)))
end

Unitful and QuadGK don't have each other as dependencies, so it would not be possible to extend either without adding more dependencies.

Do you think it would be beneficial to create a new julia package (like UnitfulQuad or something similar) to make Unitful talk nicely with QuadGK, or do you have any suggestions on how to proceed? I think out of the box numerical integration with units is a strong selling point for Julia!

@sostock
Copy link
Collaborator

sostock commented Dec 13, 2022

There is already a package that is supposed to handle compatibility between Unitful and QuadGK. It is called UnitfulIntegration. However, it hasn’t been updated in a while and is not compatible with Unitful 1.x. My suggestion would be to bring UnitfulIntegration up to date to support the current versions of QuadGK and Unitful.

For Julia ≥ 1.10 1.9, we could add the necessary methods as a package extension (JuliaLang/julia#47695). UnitfulIntegration would then no longer be needed.

@aurelio-amerio
Copy link
Author

Alright! I'm working on updating the package and I will issue a pull request as soon as possible!

@aurelio-amerio
Copy link
Author

I have submitted a pull request on UnitfulIntegration

@stevengj
Copy link

Can you give a specific test case?

I just tried it with the latest versions (QuadGK 2.6.0 and Unitful v1.12.2) in Julia 1.8.2, and it works fine for me:

julia> using Unitful, QuadGK

julia> f(t) = 1.0u"J"
f (generic function with 1 method)

julia> quadgk(f, 0.0u"s", 10.0u"s")
(10.0 J s, 0.0 J s)

@stevengj
Copy link

It seems like there is a bug in Julia where if you do using Unitful, QuadGK it works fine, but if you do using QuadGK, Unitful it fails: JuliaLang/julia#48295

@vtjnash
Copy link
Contributor

vtjnash commented Jan 16, 2023

there are a couple things here that are forbidden to generated functions.

@generated function cachedrule(::Type{T}, n::Integer) where {T<:AbstractQuantity}
    TF = typeof(float(real(one(T))))
    cache = haskey(rulecache, TF) ? rulecache[TF] : (rulecache[TF] = Dict{Int,NTuple{3,Vector{TF}}}())
    :(haskey($cache, n) ? $cache[n] : ($cache[n] = kronrod($T, n)))
end
  1. calling one is UB, since it is expected that the user may overload it with new methods
  2. accessing mutable state (e.g. rulecache here) is UB, as it may run simultaneously on multiple threads or on no threads at all in this process

For addressing 1, the solution is simply to move that code outside of the generated function, so that the argument to the generated function is TF instead of T

@sostock
Copy link
Collaborator

sostock commented Jan 16, 2023

Infinite bounds do not work, even with using Unitful, QuadGK:

julia> using Unitful, QuadGK

julia> quadgk(x->exp(-x/(1.0u"m")), 0.0u"m", Inf*u"m", atol=0.0u"m")
ERROR: DomainError with Inf m:
integrand produced NaN m in the interval (0.0 m, Inf m)
Stacktrace:
[...]

@stevengj
Copy link

Update: this turned out to be a bug in the use of @generated in QuadGK, should be closed by JuliaMath/QuadGK.jl@1959d34 (I'll tag a release soon), so this issue can be closed I think?

@stevengj
Copy link

Closed by JuliaRegistries/General#75775

@sostock
Copy link
Collaborator

sostock commented Jan 17, 2023

Infinite bounds still don’t work:

(jl_s3h7TF) pkg> st
Status `/tmp/jl_s3h7TF/Project.toml`
  [1fd47b50] QuadGK v2.7.0 `https://github.com/JuliaMath/QuadGK.jl#master`
  [1986cc42] Unitful v1.12.2

julia> using Unitful, QuadGK

julia> quadgk(x->exp(-x/(1.0u"m")), 0.0u"m", Inf*u"m")
ERROR: DomainError with Inf m:
integrand produced NaN m in the interval (0.0 m, Inf m)
Stacktrace:
[...]

@stevengj
Copy link

The problem with infinite bounds is a completely unrelated problem. I would suggest closing this issue in favor of a more specialized issue JuliaMath/QuadGK.jl#64

@stevengj
Copy link

Should be fixed now, in any case. On master (soon to be version 2.7.0):

julia> quadgk(x->exp(-x/(1.0u"m")), 0.0u"m", Inf*u"m")
(1.0 m, 4.507383379289404e-11 m)

@sostock
Copy link
Collaborator

sostock commented Jan 18, 2023

Great, thanks!

@sostock sostock closed this as completed Jan 18, 2023
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

No branches or pull requests

5 participants