Take `is_inplaceable_destination` seriously #577

mcabbott · 2022-08-25T01:04:07Z

To be useful, this function should return true only when it is certain that the array can be written into. A false false costs one allocation, but a false true will lead to an error. At present, it guesses wrong for wrappers like ReadOnlyArrays.

Commit c3bf4b0 is just fixing tests to pass on 1.9, could be pulled out.

codecov-commenter · 2022-08-25T01:07:35Z

Codecov Report

Merging #577 (3d4acc8) into main (fbb4936) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
+ Coverage   93.15%   93.18%   +0.03%     
==========================================
  Files          15       15              
  Lines         891      895       +4     
==========================================
+ Hits          830      834       +4     
  Misses         61       61

Impacted Files	Coverage Δ
src/ChainRulesCore.jl	`100.00% <ø> (ø)`
src/accumulation.jl	`97.22% <100.00%> (ø)`
src/projection.jl	`97.34% <0.00%> (+0.04%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

mcabbott · 2022-08-25T05:06:24Z

src/accumulation.jl

+is_inplaceable_destination(::DenseArray)= true
+is_inplaceable_destination(::DenseArray{<:Integer}) = false


The intention was to allow for CuArrays. But perhaps this is unsafe, as:

julia> Base.Experimental.Const([1,2]) isa DenseArray true julia> subtypes(DenseArray) 13-element Vector{Any}: Array Base.CodeUnits Base.Experimental.Const CUDA.Const CUDA.CuDeviceArray GPUArrays.AbstractDeviceArray GPUArraysCore.AbstractGPUArray Metal.Const Metal.MtlLargerDeviceArray MtlDeviceArray Random.UnsafeView SharedArrays.SharedArray SparseArrays.CHOLMOD.Dense

Suggested change

is_inplaceable_destination(::DenseArray)= true

is_inplaceable_destination(::DenseArray{<:Integer}) = false

is_inplaceable_destination(::Array)= true

is_inplaceable_destination(:: Array{<:Integer}) = false

Or perhaps:

Suggested change

is_inplaceable_destination(::DenseArray)= true

is_inplaceable_destination(::DenseArray{<:Integer}) = false

is_inplaceable_destination(x::DenseArray)= ismutable(x)

is_inplaceable_destination(x::DenseArray{<:Integer}) = false

I think to be truly safe we much only accept Array
Since anyone can subtype DenseArray, and overload setindex however they want

A pity to miss CuArrays. (Although previous version also marks them non-writable.) But agree that if the goal is always to fail on the safe side, then is has to be Array.

mzgubic

As far as I understand this PR will make some things work, and other things silently slower, because it will make them not inplaceable. Do we know how big the performance drop will be, and how widespread it will be? I imagine it should be fine as long as we cover most commonly used types here?

src/accumulation.jl

Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>

mcabbott · 2022-08-25T12:19:27Z

The cost is like copy(x). I don't know how widely used this is.

I think its real intended use is for InplaceabeThunk which, as far as I know, is not in use at all, since + always un-thunks. Are there any known users of that?

The goal is more to make this function safe to use in code which may meet unknown types. For this it needs to fail to the side which doesn't error, that's much more important than guessing correctly more often. Some discussion of such use here: FluxML/Optimisers.jl#99

Instead of changing this function, it could be deprecated & replaced.

mcabbott · 2022-08-25T12:27:56Z

There is also a question of whether it treats eltypes and structure correctly. The present one is documented to allow Diagonal(x) .+= Diagonal(y), with the update having the same structure. A two-argument ok_to_add!(x, y) could check for you, but that won't help InplaceabeThunk... xref #393 I guess.

The PR makes it return false on integers arrays, as you can't safely write a gradient into such a thing. But it won't stop you trying to write a complex array into a real one.

oxinabox · 2022-08-26T15:22:09Z

You know if you made this PR from a branch of this repo, rather than from your fork, you could just click accept on all the nagging from the autoformatter?

oxinabox · 2022-08-26T15:24:47Z

I think its real intended use is for InplaceabeThunk which, as far as I know, is not in use at all, since + always un-thunks. Are there any known users of that?

The intended use is for in add!!.
Which Nabla does use for accumulation.
But Noone uses the latest release of Nabla (its a bit fragile TBH).

I think this will have no performance impacts on anything real

src/accumulation.jl

oxinabox · 2022-08-26T15:33:03Z

src/accumulation.jl

+    alpha = is_inplaceable_destination(parent(x))
+    beta = x.indices isa Tuple{Vararg{ Union{Integer, Base.Slice, UnitRange}}}
+    return alpha && beta


Is it clearer to do

Suggested change

alpha = is_inplaceable_destination(parent(x))

beta = x.indices isa Tuple{Vararg{ Union{Integer, Base.Slice, UnitRange}}}

return alpha && beta

is_inplaceable_destination(parent(x)) || return falsse

return x.indices isa Tuple{Vararg{Union{Integer,Base.Slice,UnitRange}}}

I will trust your judgement

I had a few variants. Maybe I don't like that this seems to imply you ought to check one before the other, but in fact it's just dealing with line length.

oxinabox · 2022-08-26T15:33:52Z

src/accumulation.jl

+    @eval is_inplaceable_destination(x::Base.$T) = is_inplaceable_destination(parent(x))
+end
+for T in [:Adjoint, :Transpose, :Diagonal, :UpperTriangular, :LowerTriangular]
+    @eval is_inplaceable_destination(x::LinearAlgebra.$T) = is_inplaceable_destination(parent(x))


a bit sad that this won't catch NamedDimsArrays or KeyedArrays for free anymore.
But those need to overload ProjectTo anyway so its fine to make them do this

That wrapper types in Base is most of what we need to deal with

Besides having something you have to opt into, the other approach I can think of would be to ask type inference. But... this doesn't seem like the package's style:

julia> Core.Compiler._return_type(setindex!, typeof(([1,2,3.0], 4.0, 2))) Vector{Float64} (alias for Array{Float64, 1}) julia> Core.Compiler._return_type(setindex!, typeof((SA[1,2,3.0], 4.0, 2))) Union{} julia> Core.Compiler._return_type(setindex!, typeof((1:3.0, 4.0, 2))) Union{} julia> which(setindex!, typeof((SA[1,2,3.0], 4.0, 2))) # their own error setindex!(a::StaticArray, value, i::Int64) @ StaticArrays ~/.julia/packages/StaticArrays/6QFsp/src/indexing.jl:3 julia> which(setindex!, typeof((1:3.0, 4.0, 2))) # not useful setindex!(A::AbstractArray, v, I...) @ Base abstractarray.jl:1374

src/accumulation.jl

oxinabox

Sensible reallly.
Only handle that which we know about.

Address remaining comments and then merge when happy

src/accumulation.jl

Co-authored-by: Frames Catherine White <oxinabox@ucc.asn.au>

mcabbott · 2022-08-27T00:47:36Z

test/tangent_types/tangent.jl

        # TODO: uncomment this once https://github.com/JuliaLang/julia/issues/35516
-        if VERSION >= v"1.8-"
-            @test haskey(Tangent{Tuple{Float64}}(2.0), 1) == true
-        else
-            @test_broken haskey(Tangent{Tuple{Float64}}(2.0), 1) == true
-        end
+        # if VERSION >= v"1.8-"
+        #     @test haskey(Tangent{Tuple{Float64}}(2.0), 1) == true
+        # else
+        #     @test_broken haskey(Tangent{Tuple{Float64}}(2.0), 1) == true
+        # end
        @test_broken hasproperty(Tangent{Tuple{Float64}}(2.0), 2) == false


This is unrelated, but was needed to make tests pass on 1.9. I don't know why it says "uncomment" but now that things are commented, it may make sense.

mcabbott · 2022-08-27T00:49:18Z

test/tangent_types/tangent.jl

-        @test_throws MethodError reverse(Tangent{Foo}(; x=1.0, y=2.0))
+        if VERSION < v"1.9-"
+            # can't reverse a named tuple or a dict
+            @test_throws MethodError reverse(Tangent{Foo}(; x=1.0, y=2.0))


Likewise needed for 1.9

This tests that you can't reverse a Tangent, but do we care that you can't? We know that it's meaningless, like the order of fields in a struct. But if it used to be an error and now works, why is that a failure?

src/accumulation.jl

mcabbott added 2 commits August 24, 2022 20:58

take is_inplaceable_destination seriously

b16b878

fix tests on 1.9

c3bf4b0

mcabbott commented Aug 25, 2022

View reviewed changes

mzgubic reviewed Aug 25, 2022

View reviewed changes

src/accumulation.jl Outdated Show resolved Hide resolved

Update src/accumulation.jl

54890b9

Co-authored-by: Miha Zgubic <mzgubic@users.noreply.github.com>

mcabbott added the inplace accumulation for things relating to inplace accumulation of gradients label Aug 25, 2022

mcabbott mentioned this pull request Aug 25, 2022

Mark some arrays as safe for accumulation #578

Closed

oxinabox reviewed Aug 26, 2022

View reviewed changes

src/accumulation.jl Outdated Show resolved Hide resolved

oxinabox reviewed Aug 26, 2022

View reviewed changes

src/accumulation.jl Outdated Show resolved Hide resolved

oxinabox approved these changes Aug 26, 2022

View reviewed changes

mcabbott commented Aug 27, 2022

View reviewed changes

src/accumulation.jl Show resolved Hide resolved

Apply suggestions from code review

3d4acc8

Co-authored-by: Frames Catherine White <oxinabox@ucc.asn.au>

mcabbott commented Aug 27, 2022

View reviewed changes

mcabbott commented Aug 28, 2022

View reviewed changes

src/accumulation.jl Outdated Show resolved Hide resolved

src/accumulation.jl Outdated Show resolved Hide resolved

src/accumulation.jl Outdated Show resolved Hide resolved

three tweaks

acefc19

mcabbott merged commit aba2fcb into JuliaDiff:main Aug 28, 2022

mcabbott deleted the safe_destination branch August 28, 2022 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take `is_inplaceable_destination` seriously #577

Take `is_inplaceable_destination` seriously #577

mcabbott commented Aug 25, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading

mcabbott Aug 25, 2022

mcabbott Aug 25, 2022

oxinabox Aug 26, 2022

mcabbott Aug 27, 2022

mzgubic left a comment

mcabbott commented Aug 25, 2022

mcabbott commented Aug 25, 2022

oxinabox commented Aug 26, 2022

oxinabox commented Aug 26, 2022

oxinabox Aug 26, 2022

mcabbott Aug 27, 2022

oxinabox Aug 26, 2022 •

edited

Loading

mcabbott Aug 26, 2022

oxinabox left a comment

mcabbott Aug 27, 2022

mcabbott Aug 27, 2022

		is_inplaceable_destination(::DenseArray)= true
		is_inplaceable_destination(::DenseArray{<:Integer}) = false

Take is_inplaceable_destination seriously #577

Take is_inplaceable_destination seriously #577

Conversation

mcabbott commented Aug 25, 2022 • edited Loading

codecov-commenter commented Aug 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzgubic left a comment

Choose a reason for hiding this comment

mcabbott commented Aug 25, 2022

mcabbott commented Aug 25, 2022

oxinabox commented Aug 26, 2022

oxinabox commented Aug 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxinabox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Take `is_inplaceable_destination` seriously #577

Take `is_inplaceable_destination` seriously #577

mcabbott commented Aug 25, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading

oxinabox Aug 26, 2022 •

edited

Loading