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

fix #12375 (fix scaling of isapprox) #12393

Merged
merged 1 commit into from
Jul 31, 2015
Merged

Conversation

stevengj
Copy link
Member

As discussed in #12375, isapprox(x,y) had default tolerances that were dimensionally incorrect: if you multiply both x and y by 10, the relative tolerance should be unchanged while the absolute tolerance should multiply by 10. The new implementation fixes this issue, and is also greatly simplified:

  • The default relative tolerance is sqrt(eps(T)) where T comes from promoting x and y, for floating-point types, or zero for other types.
  • The default absolute tolerance is zero. (Nonzero absolute tolerances must come from external information that sets an overall scale.)
  • It now works for any Number type that defines real(T) to get the corresponding real type, along with abs and isfinite and -.

@stevengj stevengj added the domain:maths Mathematical functions label Jul 30, 2015
@stevengj stevengj force-pushed the isapprox branch 4 times, most recently from 6bb3070 to 52e7171 Compare July 30, 2015 17:49
@JeffBezanson
Copy link
Sponsor Member

Interesting win32 failure.

@yuyichao
Copy link
Contributor

Hmm, this is the third time I see this happen so it might be something real.

First appears on Matt's PR after the Win64 freeze is fixed. See #12248 (comment)

@yuyichao
Copy link
Contributor

I restarted the build and the failing Win32 build is here https://ci.appveyor.com/project/StefanKarpinski/julia/build/1.0.7480/job/ft5fu5cod5w7jl77

@stevengj
Copy link
Member Author

On an unrelated note, the keywords in this function are rtol and atol. In quadgk, the corresponding keywords are reltol and abstol. Shouldn't we be consistent?

@StefanKarpinski
Copy link
Sponsor Member

While I generally like short names, I must confess that it took me a while to figure out what r and a stood for. Then again, I wasn't looking at any documentation, so there's that.

@stevengj
Copy link
Member Author

Rebased (after AbstractFloat renaming).

@stevengj
Copy link
Member Author

@StefanKarpinski, I prefer the longer names in this case. If you google "reltol" and "abstol" you'll see that these abbreviations are pretty common for relative and absolute tolerances, whereas googling "rtol" and "atol" produce completely unrelated results.

But backwards compatibility for a keyword rename seems a little tricky? I guess you could do:

function foo(...; newkw::T=val, oldkw=nothing)
     if oldkw != nothing
        newkw = oldkw::T
        depwarn("oldkw is deprecated, use newkw instead", :foo)
    end
    ...
end

?

@stevengj
Copy link
Member Author

(The rtol and atol keywords of isapprox are currently used in 10 packages: ApproxFun, CLBLAS, CLFFT, Cliffords, DSP, FactCheck, GaussianMixtures, Jumos, ReverseDiffOverload, and Smile.)

@StefanKarpinski
Copy link
Sponsor Member

I think that's as good as it gets – deprecating keywords is pretty ugly.

stevengj added a commit that referenced this pull request Jul 31, 2015
@stevengj stevengj merged commit f0328c8 into JuliaLang:master Jul 31, 2015
@timholy
Copy link
Sponsor Member

timholy commented Aug 5, 2015

One thing to point out is that isapprox(eps(), 0.0) yields true on julia 0.3 but now yields false on julia 0.4. On balance I think this is correct (in what absolute sense is 1e-16 close to 0.0?), but can yield unexpected results on comparisons like @fact a --> roughly(ifft(fft(a))) when a is an Array that has some zero entries.

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

@timholy, isapprox(eps(), 0.0) == false is the correct answer without an absolute tolerance, and there's no (remotely correct) way to set an absolute tolerance without external information.

If you are comparing two arrays, a more reasonable comparison is norm(A-B) <= reltol * max(norm(A),norm(B)) in some norm. In fact, I think it would be reasonable to define isapprox(A,B) for AbstractArray{T<:Number} using precisely this definition (plus an optional absolute tolerance, and probably a norm=vecnorm keyword to choose a different norm).

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

I would say that FactCheck's roughly function for arrays is wrong for this reason.

@timholy
Copy link
Sponsor Member

timholy commented Aug 5, 2015

Agreed.

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

Successfully merging this pull request may close these issues.

None yet

5 participants