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

@test a ≈ b atol=ε #19901

Merged
merged 5 commits into from
Jan 14, 2017
Merged

@test a ≈ b atol=ε #19901

merged 5 commits into from
Jan 14, 2017

Conversation

StefanKarpinski
Copy link
Sponsor Member

Potential syntax for #19899.

@kshyatt kshyatt added the testsystem The unit testing framework and Test stdlib label Jan 6, 2017

The `@test f(args...) key=val...` form is equivalent to writing
`@test f(args..., key=val...)` which can be useful when the expression
does is a call using infix syntax such as approximate comparisons:
Copy link
Contributor

Choose a reason for hiding this comment

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

does or is, not both

@StefanKarpinski StefanKarpinski force-pushed the sk/testkws branch 2 times, most recently from 9530ca1 to 205ed30 Compare January 6, 2017 18:15
@StefanKarpinski StefanKarpinski changed the title Make @test a ≈ b atol=ε mean @test ≈(a, b, atol=ε) @test a ≈ b atol=ε Jan 6, 2017
@StefanKarpinski StefanKarpinski force-pushed the sk/testkws branch 2 times, most recently from 739035e to 29ff301 Compare January 6, 2017 19:23
# nothing.
isdefined(:test_approx_eq_modphase) ||
function test_approx_eq_modphase{S<:Real,T<:Real}(
a::StridedVecOrMat{S}, b::StridedVecOrMat{T}, err=nothing)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than copy-and-paste this into two files, can you just put it in a separate file and include it?

Copy link
Contributor

Choose a reason for hiding this comment

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

a few packages are calling this, so better to deprecate it

@stevengj
Copy link
Member

stevengj commented Jan 6, 2017

Does this work with @test a ≈ b ≈ c atol=x? We may need to define an isapprox(x,y,z...; kws) method.

@TotalVerb
Copy link
Contributor

TotalVerb commented Jan 8, 2017

We don't currently call isapprox with more than two arguments. :(a ≈ b ≈ c) parses as a chained comparison.

@test a ≈ b atol=ε

This is equivalent to the uglier test `@test ≈(a, b, atol=ε)`.
It is an error or supply more than one expression unless the first
Copy link
Contributor

Choose a reason for hiding this comment

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

"an error to supply"

@@ -912,6 +953,8 @@ end
#-----------------------------------------------------------------------
# Legacy approximate testing functions, yet to be included

# BEGIN TODO: deprecated in 0.6, delete in 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be a major pain to deal with deprecations if they were all done this way. please don't encourage this, move the code to deprecated.jl with an eval into the right module

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Otoh, it would avoid a ton of conflicts in base/deprecated.jl.

@tkelman tkelman added the needs news A NEWS entry is required for this change label Jan 8, 2017
@StefanKarpinski
Copy link
Sponsor Member Author

In light of #19936, this becomes even more useful. Going to make some improvements and then merge this once CI passes.

@tkelman
Copy link
Contributor

tkelman commented Jan 13, 2017

with the deprecations in the right place, please

@StefanKarpinski
Copy link
Sponsor Member Author

yeah, yeah

Note that tolerance of the first Anscombe's quartet test in
test/linalg/generic.jl had to be loosened from 10e-5 to 15e-5
because ≈ uses the norm of the difference instead of the maximum
absolute difference, which is a stricter test.

Closes #19899.
@StefanKarpinski
Copy link
Sponsor Member Author

AppVeyor failure is the intermittent Pkg test thing, unrelated.

@StefanKarpinski
Copy link
Sponsor Member Author

I'll leave this for review a while longer, but as far as I'm concerned, this is good to merge.

@tkelman
Copy link
Contributor

tkelman commented Jan 14, 2017

in order to not change what module the deprecations are in, should eval them into Base.Test

@StefanKarpinski
Copy link
Sponsor Member Author

FFS. That's why I didn't move them in the first place. Evaling them into Base is so much uglier.

@tkelman
Copy link
Contributor

tkelman commented Jan 14, 2017

less ugly than having them scattered all over the place. ugliness in deprecated.jl isn't much of a problem

@StefanKarpinski
Copy link
Sponsor Member Author

Well, I'm going to merge this and you can fix it because I'm getting on a plane in the morning and can't do anything for a good while.

@StefanKarpinski StefanKarpinski merged commit e9380c0 into master Jan 14, 2017
@StefanKarpinski StefanKarpinski deleted the sk/testkws branch January 14, 2017 02:06
@tkelman
Copy link
Contributor

tkelman commented Jan 14, 2017

It shouldn't be on master if it's broken. Would have rather fixed it here.

6 days ago
move the code to deprecated.jl with an eval into the right module

@stevengj
Copy link
Member

Unfortunately, this doesn't work with chained comparisons:

julia> @test x  y  z atol=0.1
ERROR: LoadError: invalid test macro call: @test x  y  z atol = 0.1
Stacktrace:
 [1] error at ./error.jl:33 [inlined]
 [2] test_expr!(::String, ::Expr, ::Expr, ::Vararg{Expr,N} where N) at /Users/stevenj/Documents/Code/julia/usr/share/julia/site/v0.7/Test/src/Test.jl:278
 [3] @test(::LineNumberNode, ::Module, ::Any, ::Vararg{Any,N} where N) at /Users/stevenj/Documents/Code/julia/usr/share/julia/site/v0.7/Test/src/Test.jl:301
in expression starting at REPL[5]:1

Just ran into this in #24159.

@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants