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

add isapprox for arrays, with ≈ and ≉ synonyms #12472

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Aug 5, 2015

As discussed in #12393, this adds an isapprox(x,y; rtol=sqrt(eps), atol=0, norm=vecnorm) function for arrays, analogous to isapprox for numbers. The definition is exactly the same except that abs is replaced by norm, and you can specify a norm via the norm keyword argument.

Rationale: this is the most natural extension of isapprox to arrays and shares the same definition, is a useful function for arrays (especially for test programs), and people often get it wrong (e.g. FactCheck.jl arguably got it wrong in their roughly function as discussed in #12393, which checks isapprox pointwise — this is almost always the wrong thing to do).

This PR also adds synonyms (U+2248, LaTeX \approx) and (U+2249, LaTeX \napprox) for isapprox and !isapprox, respectively. The official definitions of these symbols ("almost equal to") and the LaTeX abbreviations both mirror our isapprox function, so it is not really reasonable for people to define these to be any other function (and if you do need other notions of approximate equality there are other available Unicode operators, e.g. U+2245). And the binary operator is a lot more readable than isapprox (as long as you are satisfied with the default tolerances).

@stevengj stevengj added linear algebra Linear algebra unicode Related to unicode characters and encodings labels Aug 5, 2015
@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

As an added benefit, now if you do @test x ≈ y or @test x ≉ y, the @test macro treats it like any other comparison (because these parse as :comparison expressions) and prints the operands for test failures. This removes a lot of the need for @test_approx_eq, which I always found ugly.

And you can also chain isapprox comparisons with other comparisons, e.g. a ≤ x ≈ y ≤ b.

@tkelman
Copy link
Contributor

tkelman commented Aug 5, 2015

CI is screwed up with build number collisions (appveyor/ci#353) and a mac freeze, can you rebase and force push this branch again?

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

Thanks, @tkelman. I needed to update NEWS anyway.

@StefanKarpinski
Copy link
Member

The thing that bothers me about this is that it seems very common to need to tweak the behavior of isapprox. As soon as you need to do that the nice syntax can't be used. At least now that we don't automatically import operators, you can define your own operator, but it seems to me that it might be better to make it easy to do that – i.e. write something like @approx reltol=r abstol=a which defines in the current scope with the given relative and absolute tolerances. If we could make isapprox smart enough that it's rare to need to tweak it, then just letting be an alias seems ok.

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

@StefanKarpinski, if you look at our own test/ directory, it looks like > 90% of our @test_approx_eq calls use the default tolerance (as opposed specifying the tolerance via @test_approx_eq_eps). So, your claim seems empirically false to me.

@stevengj
Copy link
Member Author

stevengj commented Aug 5, 2015

I also just grepped for all usages of isapprox in registered packages and, again, > 90% of the usages employ the default tolerances.

@StefanKarpinski
Copy link
Member

Perhaps, but then why do we always seem to have random tests that fail because their tolerances are too tight? cc: @jiahao, @andreasnoack (who probably have the best sense of this)

@pao
Copy link
Member

pao commented Aug 5, 2015

We (read: @stevengj) did just change the default tolerances to Better Values, so perhaps this is less of an issue now?

@StefanKarpinski
Copy link
Member

I'd be thrilled be wrong about this, I just thought it worth bringing up as a concern.

@jiahao
Copy link
Member

jiahao commented Aug 9, 2015

@StefanKarpinski's observation is closely related to #5605 - many of the linear algebra tests (and probably tests for other numeric features too) use very loose tolerances that are not theoretically rigorous, and so in many cases are severe overestimates but in some other cases may be underestimates.

@stevengj
Copy link
Member Author

Rebased.

@StefanKarpinski
Copy link
Member

Since you guys are the ones who actually use isapprox often, whatever you decide is fine by me – this certainly does look nicer.

@stevengj
Copy link
Member Author

@jiahao, should I go ahead and merge once the tests are green?

@jiahao
Copy link
Member

jiahao commented Aug 10, 2015

Sure

@stevengj
Copy link
Member Author

(My feeling is that most @test_approx_eq tests employ the default tolerances because that they are checking for gross errors, not subtle roundoff sensitivities. If a test is accurate to half the significant digits, then analyzing the accuracy of the remaining digits involves a delicate error analysis that is often not available.)

stevengj added a commit that referenced this pull request Aug 10, 2015
add isapprox for arrays, with ≈ and ≉ synonyms
@stevengj stevengj merged commit 80be6dc into JuliaLang:master Aug 10, 2015
@@ -2935,11 +2935,18 @@ cis
doc"""
```rst
::
isapprox(x::Number, y::Number; rtol::Real=sqrt(eps), atol::Real=0)
isapprox(x, y; rtol::Real=sqrt(eps), atol::Real=0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

signature may need to be updated to match this in the RST docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 10, 2015
stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 10, 2015
stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 10, 2015
stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 11, 2015
add AbstractFloat (fix #121)
stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 11, 2015
add AbstractFloat (fix #121)
stevengj added a commit to JuliaLang/Compat.jl that referenced this pull request Aug 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra unicode Related to unicode characters and encodings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants