Skip to content

add!! with Thunk - #348

Merged
oxinabox merged 1 commit into
masterfrom
ox/ipthunk
May 10, 2021
Merged

oxinabox merged 1 commit into
masterfrom
ox/ipthunk

Conversation

@oxinabox

@oxinabox oxinabox commented May 7, 2021

Copy link
Copy Markdown
Member

Its not as good as add!!(::AbstractArray, ::InplacableThunk)
but if it can be inplace then it still saves 1 allocation (InplaceableThunk woulds save 2)/

@oxinabox oxinabox added the inplace accumulation for things relating to inplace accumulation of gradients label May 7, 2021
@codecov-commenter

codecov-commenter commented May 7, 2021

Copy link
Copy Markdown

Codecov Report

Merging #348 (aa24438) into master (3352d15) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   90.01%   90.03%   +0.01%     
==========================================
  Files          14       14              
  Lines         541      542       +1     
==========================================
+ Hits          487      488       +1     
  Misses         54       54              
Impacted Files Coverage Δ
src/accumulation.jl 97.14% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3352d15...aa24438. Read the comment docs.

@mzgubic mzgubic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread src/accumulation.jl
end
end

add!!(x::AbstractArray, y::Thunk) = add!!(x, unthunk(y))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this method didn't exist, add!!(abstract_array, thunk) would just dispatch to the fallback. Now that it does exist, it dispatches to the method below, which saves one accumulation. It's late and my brain hurts and I had to write this down to convince myself that it makes sense.

@oxinabox
oxinabox merged commit 1a37f0a into master May 10, 2021
@oxinabox
oxinabox deleted the ox/ipthunk branch May 10, 2021 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inplace accumulation for things relating to inplace accumulation of gradients

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants