Skip to content

should add!!(x, t::InplacableThunk) make sure that x is mutable before calling t.add! #232

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

Closed
oxinabox opened this issue Oct 14, 2020 · 1 comment · Fixed by #234
Closed
Assignees
Labels
inplace accumulation for things relating to inplace accumulation of gradients

Comments

@oxinabox
Copy link
Member

oxinabox commented Oct 14, 2020

Came up in JuliaDiff/ChainRules.jl#287

I suspect add!! should take responsiblity as to if it is appropriate to call the InplaceableThunk's add! field.

Its fairly easy for us to setup a baseline set of mutable types:

"returns true if `x .= y` will work to mutate `x`"
is_broadcast_mutable{::Array}} = true
is_broadcast_mutable{::SparseVector}} = true
is_broadcast_mutable{::SparseMatrixCSC}} = true
is_broadcast_mutable{::BitArray} = true  # probably don't need this one
function is_broadcast_mutable(x::AbastractArray)
    p = parent(x)
    p === x && return false  # did not find a mutable parent
    return is_broadcast_mutable(p)
end

is_broadcast_mutable(::Any) = false

ArrayInterface has something like this, but gets it wrong (JuliaArrays/ArrayInterface.jl#77)
and we don't want the dependency

Other types can specialize add!! themselves; with what ever way they like to be mutated.

@oxinabox oxinabox added the inplace accumulation for things relating to inplace accumulation of gradients label Oct 14, 2020
@oxinabox oxinabox changed the title should add!(x, t::InplacableThunk) make sure that x is mutable before calling t.add! should add!!(x, t::InplacableThunk) make sure that x is mutable before calling t.add! Oct 14, 2020
@oxinabox oxinabox assigned oxinabox and unassigned willtebbutt Oct 15, 2020
@oxinabox
Copy link
Member Author

had a chat with @nickrobinson251 on this.
We should do this:

  • It makes the interface of InplaceThunk.add! clearer: it only has to handle mutating cases
  • It avoids adding lots of functions like _subtract!! into ChainRules etc add rule for matrix negation ChainRules.jl#287, that each individually handle this
  • We already kind of do this for the case of the first argument being Zero() anyway.
  • Downside is it is a bit more magical, in the way that add!! is already a bit magical: it decides behind the scenes if to be inplace or not
  • We should also add a thing in debug mode that checked if the InplaceThunk.add!! did infact mutate.

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 a pull request may close this issue.

2 participants