-
-
Notifications
You must be signed in to change notification settings - Fork 28
Change initialization of tolerances of fixed point iterations #95
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
Conversation
Co-Authored-By: devmotion <[email protected]>
abstol = integrator.opts.abstol | ||
if typeof(abstol) <: Number | ||
fixedpoint_abstol_internal = abstol | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether this specialization is required. To me it seems recursivecopy(::Number)
falls back to deepcopy
which both give a more complex output with e.g. @code_native recursivecopy(3)
and @code_native deepcopy(3)
than @code_native copy(3)
? Should we just define recursivecopy(x::Number) = copy(x)
in RecursiveArrayTools which by default just returns x
(https://github.com/JuliaLang/julia/blob/master/base/number.jl#L88)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. There shouldn't be a need to deepcopy
a number anymore. This could improve performance in some places as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure Measurements.jl still works after changing to copy
. It should.
This PR changes how the tolerances of fixed point iterations are initialized. If unspecified, the error tolerances of the integrator are used without any conversions - the correct types are supposed to be set by the logic in OrdinaryDiffEq (https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/solve.jl#L112-L134). If they are specified, then it is only ensured that they are real numbers (similar to https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/solve.jl#L121 and https://github.com/JuliaDiffEq/OrdinaryDiffEq.jl/blob/master/src/solve.jl#L133). Hence if units are used and tolerances are specified they always have to be given with correct units. Currently unitless numbers were converted to the correct units but in my opinion we shouldn't do that since it is not transparent to the user.
This PR, however, has bigger implications: when used with Flux currently
fixedpoint_reltol_internal
is initialized asTrackedReal{TrackedReal{Float64}}
which is incorrect and causes a stack overflow. With this PR the following code (modification of JuliaLang/www.julialang.org#274) runs without any issues: