-
Notifications
You must be signed in to change notification settings - Fork 31
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
Start testing reverse mode #254
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 55.11% 55.29% +0.17%
==========================================
Files 28 28
Lines 2892 2917 +25
==========================================
+ Hits 1594 1613 +19
- Misses 1298 1304 +6 ☔ View full report in Codecov by Sentry. |
Let's rename it to FYI: he name "hobbit" for this is a reference to to
Hobbit == There and Back Again |
I tend to do what you are doing |
src/stage1/generated.jl
Outdated
BB = zero(a) | ||
BB[inds...] = unthunk(Δ) | ||
view(BB, inds...) .+= unthunk(Δ) |
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.
why is this different?
0 .+ Δ = Δ
at least assuming they are the same size
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.
Ah sorry, I should have added a comment here. this is for cases like
julia> gradient(sum ∘ x -> x[:,[1,1,2]], rand(3,4))[1]
3×4 Matrix{Float64}:
2.0 1.0 0.0 0.0
2.0 1.0 0.0 0.0
2.0 1.0 0.0 0.0
The view
lets one add duplicate indices
julia> x = zeros(3,4)
3×4 Matrix{Float64}:
0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0
julia> view(x, :, [1,1,2]) .+= ones(3,3)
3×3 view(::Matrix{Float64}, :, [1, 1, 2]) with eltype Float64:
2.0 2.0 1.0
2.0 2.0 1.0
2.0 2.0 1.0
julia> x
3×4 Matrix{Float64}:
2.0 1.0 0.0 0.0
2.0 1.0 0.0 0.0
2.0 1.0 0.0 0.0
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.
oh cool trick. I think last time i needed to handle that my solution was way more complicated.
I think we should merge this more or less as is and then can continue with follow up PRs to add more tests / fix existing broken ones. |
Yes, that sounds good! I can also open another PR with the |
It's fine for now. |
Adds tests for
Base
taken from Zygote. There will be quite a few more tests coming in, probably in a new PR (essentially planning to updated/split #73 into smaller MRs). Many of the added tests are currently broken (with the error message in the comment above the broken test).Two main takeaways so far are: kwargs seem to break tests with one of two errors:
and then there are a few tests that return incorrect gradients.
Is there a good place to keep the main problems of the broken tests?
In #73 the
pullback
function from Zygote is kept, should we keep it or rewrite those tests in a different way?