-
Notifications
You must be signed in to change notification settings - Fork 15
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
Avoid nesting testsets in test_rule #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a version bump.
### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using undocumented functions a code smell of some sort? I think it indicates that either:
Test
package 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@test
print 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:25
before:
Foo.2: Test Failed at /Users/oxinabox/.julia/packages/ChainRulesTestUtils/0XyYu/src/check_result.jl:19
In general having the
testset
breakdown 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).