Conversation
| ### helpers for printing in log messages etc | ||
| _string_typeof(x) = string(typeof(x)) | ||
| _string_typeof(xs::Tuple) = join(_string_typeof.(xs), ",") | ||
| _string_typeof(x::PrimalAndTangent) = _string_typeof(primal(x)) # only show primal |
There was a problem hiding this comment.
| _string_typeof(x::PrimalAndTangent) = _string_typeof(primal(x)) # only show primal | |
| _string_typeof(x::PrimalAndTangent) = "$(_string_typeof(primal(x))) ⊢ $(_string_typeof(tangent(x)))" |
Are we ever interested in the tangent type? Probably not?
There was a problem hiding this comment.
yeah i considered that. Note that that would display things like Vector{Float64} ⊢ NoTangent() which is a bit of a play on words since the ⊢ operator doesn't actually accept types.
I think we can leave it as is (displaying just primals) for now, and consider adding that later.
| Test.test_expr!("@test_msg msg", ex, kws...) | ||
|
|
||
| result = Test.get_test_result(ex, __source__) | ||
| return :(Test.do_test($result, $ExprAndMsg($(string(ex)), $(esc(msg))))) |
There was a problem hiding this comment.
Is using undocumented functions a code smell of some sort? I think it indicates that either:
Testpackage should be easier to extent/modify
or- We are not achieving our goal in the right way with the tools available
I am leaning towards 1) from other experience with Test. On the other hand, I don't fully understand this PR (namely the test_msg macro). What do get_test_result and do_test actually do?
There was a problem hiding this comment.
AFAIK it is 1) I will add a comment. I don't really understand what they do either, just that this is the thing that is needed.
It is kinda copied directly from the Test stdlib.
This whole thing with ExprAndMsg is a huge hack.
There was a problem hiding this comment.
Yeah, fair enough. LGTM just needs a version bump
Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>
make rand_tangent of struct with no perturbable fields return DoesNotExist
Combined with the other PRs i have open this should close #146.
We don't nest testsets any more, except in
test_scalar(and I don't think thats much of a problem just cos scalars rarely fail their pullbacks are simple).Instead we use a hack to make
@testprint a message on failure.and then we plumb that around as needed.
Example of one of the place we would nest a test set before. Mocking up the outer set just for this demo
vs before:
which doesn't look like much and i guess probably wasn't a great example (someone should find and try some others)
But it is nice that the error now at the top is about what is going on
now:
rrule for foo at bar: Test Failed at /Users/oxinabox/JuliaEnvs/ChainRulesWorld/ChainRules.jl/dev/ChainRulesTestUtils/src/check_result.jl:25before:
Foo.2: Test Failed at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19In general having the
testsetbreakdown sounds nice.But it isn't actually useful
What you really want is the error message:
test/...which triggered it (so you can see exactly how it was configured).Everything else is kinda noise.
but that info is no where near the testset summary at the end.
It also tweaks the content of the testset title that was set in #154 so that it also prints just primal type even if you passed in PrimalAndTangent. (this avoids printingthat long
ChainrulesCore.PrimalAndTangent{Float64, Float64}type).