Overhall equality checks, to give better failure messages #57

oxinabox · 2020-10-05T18:30:28Z

I wanted to add accumulation checks,
but to do that
I felt it was good to refactor away from using isapprox as much as we do,
I am not entirely sure this PR is a great idea,
the accumulation stuff is easy enough to put in another PR, if we don't like the rest of it.

This PR is a great idea.
To deal with giving useful error messages an not things about thunks not being equal.
and getting rid of all the hacks with extern, collect, and isapprox.
effectively closes #7 as we will be able to delete isapprox on differentials with the next major release

oxinabox · 2020-10-05T18:32:05Z

src/iterator.jl

+# To make it a valid differential: needs at very least `zero` and `+`
+Base.zero(::Type{<:TestIterator}) = Zero()
+function Base.:+(iter1::TestIterator{T,IS,IE}, iter2::TestIterator{T,IS,IE}) where {T,IS,IE}
+    return TestIterator{T,IS,IE}(map(+, iter1, iter2))
+end


We were using this like a differential, without these being defined,
which ment that testing accumulation errored.

Hm you're right. The idea with TestIterator is for it to behave like some generic iterator. For most iterators, we will not be able to define e.g. zero(::SomeIterator) without type piracy, so maybe the type of TestIterator's differential should not be TestIterator but instead a Composite?
But then again, I guess most iterators won't have isapprox defined on them, so we're already not super consistent here.

src/check_result.jl

oxinabox · 2020-10-05T18:34:00Z

src/testers.jl

-                frule((Zero(), Δx), f, z; fkwargs...)[2],
+            check_equal(
+                frule((Zero(), real(Δx)), f, z; fkwargs...)[2]::Number,
+                frule((Zero(), Δx), f, z; fkwargs...)[2]::Number,


scalar rules shout return Numbers.
but maybe this should be moved out of this part of the test and be somewhere else
tested explictly.

Yes, I agree that this should have an explicit isa Number test.

I think we can leave this for now, I have removed this from the test

src/testers.jl

oxinabox · 2020-10-05T18:37:49Z

I don't really love this PR. though I like it more than the code it replaces i think.
I might sit on if for a few days see if anyone else has good input

src/check_result.jl

sethaxen

I haven't followed the accumulation PR(s?), so take this review with that caveat. In general I like the introduction of more human-readable errors (can you actually add a test that check_equal produces human-readable errors for a case where isapprox would not have?), and I don't see a problem with the accumulation part (can you add a test for the testers for an accumulating function?)

sethaxen · 2020-10-06T16:54:55Z

src/iterator.jl

+# To make it a valid differential: needs at very least `zero` and `+`
+Base.zero(::Type{<:TestIterator}) = Zero()
+function Base.:+(iter1::TestIterator{T,IS,IE}, iter2::TestIterator{T,IS,IE}) where {T,IS,IE}
+    return TestIterator{T,IS,IE}(map(+, iter1, iter2))
+end


Hm you're right. The idea with TestIterator is for it to behave like some generic iterator. For most iterators, we will not be able to define e.g. zero(::SomeIterator) without type piracy, so maybe the type of TestIterator's differential should not be TestIterator but instead a Composite?
But then again, I guess most iterators won't have isapprox defined on them, so we're already not super consistent here.

sethaxen · 2020-10-06T16:56:42Z

src/testers.jl

-                frule((Zero(), Δx), f, z; fkwargs...)[2],
+            check_equal(
+                frule((Zero(), real(Δx)), f, z; fkwargs...)[2]::Number,
+                frule((Zero(), Δx), f, z; fkwargs...)[2]::Number,


Yes, I agree that this should have an explicit isa Number test.

src/testers.jl

src/check_result.jl

oxinabox · 2020-12-11T20:18:30Z

I have come back to this because i an sick of bad messages around thunks
so it is rebased and has test now
@sethaxen can you rereview

Example change:

julia> oldcheck(log(-1.0 - im), k)
Test Failed at REPL[95]:1
  Expression: isapprox(actual, expected)
   Evaluated: isapprox(0.34657359027997264 - 2.356194490192345im, Thunk(var"#217#218"()))
ERROR: There was an error during testing

julia> check(log(-1.0 - im), k)
Test Failed at /Users/oxinabox/JuliaEnvs/ChainRulesWorld/ChainRulesTestUtils.jl/src/check_result.jl:18
  Expression: isapprox(actual, expected; kwargs...)
   Evaluated: isapprox(0.34657359027997264 - 2.356194490192345im, 0.34657359027997264 + 2.356194490192345im)
ERROR: There was an error during testing

oxinabox · 2020-12-11T20:35:35Z

src/testers.jl

-            )
+            _, real_tangent = frule((Zero(), real(Δx)), f, z; fkwargs...)
+            _, embedded_tangent = frule((Zero(), Δx), f, z; fkwargs...)
+            _check_equal(real_tangent, embedded_tangent; isapprox_kwargs...)
        end
    end
    if z isa Complex


I want to move the 4 sections of this function that happen only for complex out into 2 helper functions.
(one for forward on one for reverse)
but that should be a follow up PR as it will make this hard to read.

nickrobinson251 · 2020-12-11T20:38:00Z

Does this close #7? I think probably, but not sure because i've just given it a quick look.

Also does it make any difference to #56?

oxinabox · 2020-12-11T20:38:59Z

src/check_result.jl

+are shown on failures.
+All keyword arguments are passed to `isapprox`.
+"""
+function _check_equal(


I wonder if we should export this, (and rename it)
for people who need to write special tests?
Might make it feel more safe to remove isapprox as there is a handy subsitution

As I think we're probably committed to maintaining some functionality of this form, I agree that we should export it.

src/check_result.jl

test/check_result.jl

src/check_result.jl

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

src/check_result.jl

oxinabox · 2020-12-11T20:56:52Z

src/iterator.jl

-) where {T1,T2,IS,IE}
-    return isapprox(iter1.data, iter2.data; kwargs...)
-end
-


Technically this is breaking, but I am pretty sure they are not used except in this package.
But I could keep them and move them to deprecated, do you think?

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

oxinabox · 2020-12-11T22:09:16Z

Does this close #7? I think probably, but not sure because i've just given it a quick look.

Yes, this closes #7 (technically would need to wait for next breaking change when isapprox on all the differnetials is deleted but its not used internally any more)

Also does it make any difference to #56?

#56 was actually closed by the follow up to this PR that was more minimal #59, but I forgot to close it

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

src/check_result.jl

oxinabox commented Oct 5, 2020

View reviewed changes

src/check_result.jl Show resolved Hide resolved

oxinabox commented Oct 5, 2020

View reviewed changes

src/testers.jl Outdated Show resolved Hide resolved

oxinabox commented Oct 5, 2020

View reviewed changes

src/testers.jl Outdated Show resolved Hide resolved

oxinabox commented Oct 5, 2020

View reviewed changes

src/check_result.jl Outdated Show resolved Hide resolved

oxinabox requested review from mattBrzezinski, nickrobinson251 and sethaxen and removed request for sethaxen October 6, 2020 12:51

oxinabox mentioned this pull request Oct 6, 2020

Minimal Inplace accumulation testing #59

Merged

sethaxen requested changes Oct 6, 2020

View reviewed changes

oxinabox mentioned this pull request Oct 12, 2020

Correct inplace for complex BLAS and fix some accumulation against nondifferentails JuliaDiff/ChainRules.jl#279

Merged

oxinabox mentioned this pull request Nov 16, 2020

rrule_test requires returned composite objects to be collectable #74

Closed

oxinabox changed the title ~~RFC: overhall equality checks and add accumulation checks~~ RFC: overhall equality checks, to give better failure messages Dec 11, 2020

overhall equality checks and add accumulation checks

ef5f728

oxinabox force-pushed the ox/inplace branch from 516d065 to ef5f728 Compare December 11, 2020 14:25

oxinabox added 2 commits December 11, 2020 17:20

remove all uses of isapprox and use _checkequal

f4b1b98

Test _check_equals

becabe1

oxinabox changed the title ~~RFC: overhall equality checks, to give better failure messages~~ Overhall equality checks, to give better failure messages Dec 11, 2020

Handle composites with some zeros

305022e

oxinabox requested a review from sethaxen December 11, 2020 20:32

oxinabox commented Dec 11, 2020

View reviewed changes

willtebbutt reviewed Dec 11, 2020

View reviewed changes

src/check_result.jl Outdated Show resolved Hide resolved

willtebbutt reviewed Dec 11, 2020

View reviewed changes

test/check_result.jl Outdated Show resolved Hide resolved

willtebbutt reviewed Dec 11, 2020

View reviewed changes

src/check_result.jl Outdated Show resolved Hide resolved

oxinabox and others added 3 commits December 11, 2020 20:44

move isapprox into deprecated file

33176f7

remove isapprox from TestIterator

aa362e6

spacing

175c001

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

oxinabox commented Dec 11, 2020

View reviewed changes

src/check_result.jl Outdated Show resolved Hide resolved

oxinabox commented Dec 11, 2020

View reviewed changes

Update test/check_result.jl

fb857af

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

typo

bdc621b

Co-authored-by: willtebbutt <wt0881@my.bristol.ac.uk>

oxinabox commented Dec 14, 2020

View reviewed changes

src/check_result.jl Outdated Show resolved Hide resolved

Only alll _check_equal on Zero not AbstractZero

a973aac

willtebbutt approved these changes Dec 14, 2020

View reviewed changes

oxinabox added 3 commits December 14, 2020 18:40

remove test of DoesNotExist checkequals Zero

75bc572

rename and export check_equal

65e4667

bump version

dc1f787

oxinabox merged commit 7bbd60e into master Dec 14, 2020

This was referenced Dec 15, 2020

ChainRules rrules_tests fail on Matrix implementation JuliaArrays/BlockDiagonals.jl#49

Closed

support isapprox still #83

Merged

More Reverse Dependency Integration Test #85

Closed

nickrobinson251 mentioned this pull request Jan 23, 2021

check_equal for DoesNotExist #109

Open

Overhall equality checks, to give better failure messages #57

Overhall equality checks, to give better failure messages #57

Uh oh!

Conversation

oxinabox commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oxinabox commented Oct 5, 2020

Uh oh!

Uh oh!

sethaxen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oxinabox commented Dec 11, 2020

Uh oh!

oxinabox Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickrobinson251 commented Dec 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxinabox commented Dec 11, 2020

Uh oh!

Uh oh!

Uh oh!

oxinabox commented Oct 5, 2020 •

edited

Loading

oxinabox Dec 11, 2020 •

edited

Loading