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

type stability #36

Merged
merged 6 commits into from
Jun 7, 2019
Merged

type stability #36

merged 6 commits into from
Jun 7, 2019

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jun 7, 2019

Makes the quadgk function type-stable. Closes #21. Closes #19. Closes #15. Closes #20.

Also fixes handling of some other other number types, and in particular fixes dimensionful types (#17). Mostly supersedes https://github.com/PainterQubits/UnitfulIntegration.jl, cc @ajkeller34:

julia> using Unitful, QuadGK, Test

julia> @inferred quadgk(x -> x^2, 0u"m", 1u"m")
(0.3333333333333333 m^3, 5.551115123125783e-17 m^3)

It still doesn't completely address #13, because it only works if real(one(T)) returns the underlying real numeric type for computing the quadrature weights etc. This doesn't work for dual numbers (ForwardDiff.jl) or measurements (JuliaPhysics/Measurements.jl#31), and in general I haven't been able to think of a way to do this that works across packages.

src/evalrule.jl Outdated Show resolved Hide resolved
src/adapt.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member Author

stevengj commented Jun 7, 2019

Note that the code will now fail if the integrand returns one (or more) types when it is evaluated at the initial quadrature points, and then suddenly returns another type (that is incompatible under convert) when it is evaluated at refined points. In principle, I know how to handle this (by reallocating the segs array in adapt … see the todo). In practice, I'm doubtful that it is worth the trouble. Fixed.

@ararslan
Copy link
Member

ararslan commented Jun 7, 2019

That to me seems like something that is unlikely to occur in most user code, so I'd agree that it's not likely worth the trouble. Though perhaps a warning about it in the documentation would be warranted, in the off chance someone runs into it?

@MasonProtter
Copy link

It still doesn't completely address #13, because it only works if real(one(T)) returns the underlying real numeric type for computing the quadrature weights etc.

That's frustrating. Why does the quadrature weights calculation need real numbers?

Perhaps one thing that could be done is put in a function value and a base definition value(x::Number) = real(x) and then put a note in the documentation that if someone is trying to make their custom number type work with QuadGK, they need to define value on that number type.

ie.

QuadGK.value(x::ForwardDiff.Dual) = x.value
QuadGK.value(x::Unitful.Quantity) = x.val

@stevengj
Copy link
Member Author

stevengj commented Jun 7, 2019

I pushed a fix for type-unstable functions; it turned out to be not too hard.

@stevengj
Copy link
Member Author

stevengj commented Jun 7, 2019

Perhaps one thing that could be done is put in a function value and a base definition value(x::Number)

Then any package that wanted this to work would have to depend on QuadGK. This really needs to go into Base.

@stevengj stevengj merged commit 3a876c9 into master Jun 7, 2019
@stevengj stevengj deleted the sgj/type-stability branch June 7, 2019 22:34
@stevengj
Copy link
Member Author

Maybe packages that define new number types should define AbstractFloat(x::MyNumber) to give the raw-floating-point value? (They should define a separate float(x::MyNumber) method to convert to a floating-point version of the same type.)

@giordano
Copy link
Member

What is the "raw-floating-point value"?

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 this pull request may close these issues.

rulekey internal function undocumented and type unstable Parametrize I and E types Type unstable?
4 participants