-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gentle ping on this. If there are no good ideas then I think this is ready?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).