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

Comparison tests ZeroTangent with 0-like numbers should be true #504

Open
matbesancon opened this issue Oct 28, 2021 · 7 comments
Open

Comparison tests ZeroTangent with 0-like numbers should be true #504

matbesancon opened this issue Oct 28, 2021 · 7 comments

Comments

@matbesancon
Copy link
Contributor

Was just testing something boiling down to:
ZeroTangent() ≈ 0
Should this be accepted? Along with arrays for which the norm is 0?

@mzgubic
Copy link
Member

mzgubic commented Oct 28, 2021

Yes, though we use test_approx in ChainRulesTestUtils

julia> using ChainRulesTestUtils

julia> test_approx(ZeroTangent(), 0)
Test Passed

julia> test_approx(ZeroTangent(), zeros(2, 3))
Test Passed

@matbesancon
Copy link
Contributor Author

indeed good point. Could it be worth it to have it somewhere close to the ZeroTangent doc?

@mzgubic
Copy link
Member

mzgubic commented Oct 28, 2021

I would argue it belongs to the ChainRulesTestUtils docs, but we could certainly add a link to those.

@mcabbott
Copy link
Member

mcabbott commented Oct 28, 2021

What's the argument against defining these?

Besides being one less thing to know about, @test x ≈ 0.0 will I think give more useful information when it fails, as the macro records where it was called.

Maybe array ones are weird, in that @test x ≈ [0 0 0] normally implies you check the size as well as the contents, whereas Zero doesn't have any size of course.

Finally, x ≈ 0 is the same as x == 0 for numbers, I think. Unless you supply the tolerance. But ZeroTangent() == 0 is false, not undefined.

@matbesancon
Copy link
Contributor Author

yes I wasn't sure about arrays, I found norm(x) == 0 a good proxy

@mzgubic
Copy link
Member

mzgubic commented Oct 28, 2021

I think the reason we have test_approx is to better display the information when we have thunking involved.

compare:

julia> using ChainRulesCore

julia> using ChainRulesTestUtils

julia> using Test

julia> struct Foo end

julia> unfoo(x) = x
unfoo (generic function with 1 method)

julia> unfoo(f::Foo) = 0.0
unfoo (generic function with 2 methods)

julia> Base.isapprox(f::Foo, b) = isapprox(unfoo(f), b)

julia> @test Foo()  0.1
Test Failed at REPL[11]:1
  Expression: Foo()  0.1
   Evaluated: Foo()  0.1
ERROR: There was an error during testing

to

julia> using ChainRulesCore

julia> using ChainRulesTestUtils

julia> using Test

julia> struct Foo end

julia> unfoo(f::Foo) = 0.0
unfoo (generic function with 1 method)

julia> unfoo(x) = x
unfoo (generic function with 2 methods)

julia> ChainRulesTestUtils.test_approx(f::Foo, b) = test_approx(unfoo(f), b)

julia> test_approx(Foo(), 0.1)
Test Failed at /Users/mzgubic/JuliaEnvs/ChainRulesTestUtils.jl/src/check_result.jl:24
  Expression: isapprox(actual, expected; kwargs...)
   Evaluated: isapprox(0.0, 0.1)
ERROR: There was an error during testing

@oxinabox
Copy link
Member

We used to define .
It was surprisingly awkward.
And it gave surprisingly bad error messages.

I suspect it is also pretty invalidating, if we define it in CRC, but I haven't tested that.

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

No branches or pull requests

4 participants