-
Notifications
You must be signed in to change notification settings - Fork 112
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
Make the isapprox
implementation closer to the LinearAlgebra implementation
#719
Conversation
Co-authored-by: Sebastian Stock <[email protected]>
@sostock, PR ready for final review and merge. |
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.
There should be some tests for the new behavior. Suggestions:
@test isapprox([1.0*m, NaN*m], [nextfloat(1.0)*m, NaN*m], nans=true)
@test !isapprox([1.0*m, NaN*m], [nextfloat(1.0)*m, NaN*m], nans=false)
@test !isapprox([1.0m, 2.0m], [1.1m, 2.2m], rtol=0.05, atol=0.2m)
@test !isapprox([1.0m], [nextfloat(1.0)*m], atol = eps(0.1)*m)
If I didn’t make a mistake, all of these tests should error/fail on the old version and pass with this PR.
The scalar isapprox(::AbstractQuantity{T,D,U}, ::AbstractQuantity{T,D,U}) where {T,D,U}
method should be changed to use ustrip(absoluteunit(unit(y)), atol)
, (and tests should be added for that as well), but that is unrelated to the changes in this PR and can be done later.
After this PR is merged, I will make a new release (a minor release, since in my opinion the addition of the nans
keyword qualifies as a feature).
Co-authored-by: Sebastian Stock <[email protected]>
@sostock ready for merge |
The Unitful implementation of
isapprox(::AbstractArray, ::AbstractArray)
function has some differences compared to the LinearAlgebra implementation.Link: https://github.com/JuliaLang/julia/blob/master/stdlib/LinearAlgebra/src/generic.jl#L1879-L1891
For example, the comparison in Unitful is:
But in LinearAlgebra is:
Another difference is in the use of the
Base.rtoldefault
function.The
Base.rtoldefault(x, y, atol)
function returns the defaultrtol
only whenatol
is less than or equal to zero, otherwise it returns zero.But, in Unitful, the last argument of
Base.rtoldefault
is always zero:That is, the
rtol
value is always be different than zero, even whenatol > 0
.Below is the LinearAlgebra implementation for comparison:
The last difference is that the Unitful doesn't forward the
nans
keyword argument.These different implementations result in different behaviours:
master branch:
This PR adjusts the implementation to be closer to the LinearAlgebra implementation: