full fix to #1352 #1355
full fix to #1352 #1355
Conversation
| return du[:, 1] | ||
| end | ||
| # reduced mwe of #1352 | ||
| @test Zygote.gradient([0,0]) do x |
There was a problem hiding this comment.
Maybe use some more complicated function and some non-zero values? It happened to me before (eg in DistributionsAD) that too "nice" values and examples did not catch (all) problems.
There was a problem hiding this comment.
Sure, any suggestions? The test is mainly of the pullback of getindex(x::Buffer, ::UnitRange) which was not getting tested before (only getindex(x::Buffer, ::Int) was).
There was a problem hiding this comment.
Gentle ping on this. If there are no good ideas then I think this is ready?
There was a problem hiding this comment.
Fine by me to go ahead as-is or for someone to tweak the test. Just rebased on master for convenience.
There was a problem hiding this comment.
Given that eltype(x) eventually falls back to Any, I think this is pretty safe. We can always tweak the tests later if someone has a good idea on how to do so.
marius311
force-pushed
the
better_fix_1352
branch
from
a124adc
to
8418647
Compare
| return du[:, 1] | ||
| end | ||
| # reduced mwe of #1352 | ||
| @test Zygote.gradient([0,0]) do x |
There was a problem hiding this comment.
Given that eltype(x) eventually falls back to Any, I think this is pretty safe. We can always tweak the tests later if someone has a good idea on how to do so.
I did screw up #1350 slightly sorry, thanks for catching that in #1352 @frankschae, but @devmotion's #1353 is only a partial fix because the allocator ends up eltype
Any. Here's a full fix. I also reduced the test (this test also would have caught the error in my initial PR).