Skip to content
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

Forbid offsets #586

Merged
merged 3 commits into from
Dec 13, 2022
Merged

Forbid offsets #586

merged 3 commits into from
Dec 13, 2022

Conversation

mcabbott
Copy link
Member

@mcabbott mcabbott commented May 17, 2022

This follows LinearAlgebra in explicitly forbidding offset arrays. They don't work anyway, but this replaces a BoundsError with something deliberate:

julia> ForwardDiff.gradient(sum, OffsetArray([1,2,3], 3))  # before
ERROR: BoundsError: attempt to access 3-element OffsetArray(::Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}, 4:6) with eltype Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3} with indices 4:6 at index [1:3]
Stacktrace:
  [1] throw_boundserror(A::OffsetVector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}, Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}}, I::Tuple{UnitRange{Int64}})
    @ Base ./abstractarray.jl:703
  [2] checkbounds
    @ ./abstractarray.jl:668 [inlined]
  [3] view
    @ ./subarray.jl:177 [inlined]
  [4] maybeview
    @ ./views.jl:148 [inlined]
  [5] dotview
    @ ./broadcast.jl:1201 [inlined]
  [6] seed!(duals::OffsetVector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}, Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}}, x::OffsetVector{Int64, Vector{Int64}}, seeds::Tuple{ForwardDiff.Partials{3, Int64}, ForwardDiff.Partials{3, Int64}, ForwardDiff.Partials{3, Int64}})
    @ ForwardDiff ~/.julia/dev/ForwardDiff/src/apiutils.jl:65
  [7] vector_mode_dual_eval!
    @ ~/.julia/dev/ForwardDiff/src/apiutils.jl:36 [inlined]
  [8] vector_mode_gradient(f::typeof(sum), x::OffsetVector{Int64, Vector{Int64}}, cfg::ForwardDiff.GradientConfig{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3, OffsetVector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}, Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}}})
    @ ForwardDiff ~/.julia/dev/ForwardDiff/src/gradient.jl:106
  [9] gradient(f::Function, x::OffsetVector{Int64, Vector{Int64}}, cfg::ForwardDiff.GradientConfig{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3, OffsetVector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}, Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}}}, ::Val{true})
    @ ForwardDiff ~/.julia/dev/ForwardDiff/src/gradient.jl:0
 [10] gradient(f::Function, x::OffsetVector{Int64, Vector{Int64}}, cfg::ForwardDiff.GradientConfig{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3, OffsetVector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}, Vector{Dual{ForwardDiff.Tag{typeof(sum), Int64}, Int64, 3}}}}) (repeats 2 times)
    @ ForwardDiff ~/.julia/dev/ForwardDiff/src/gradient.jl:17
 [11] top-level scope
    @ REPL[162]:1

julia> ForwardDiff.gradient(sum, OffsetArray([1,2,3], 3))  # after
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1

@mcabbott mcabbott force-pushed the no_offsets branch 2 times, most recently from be62c4b to 6d5f6c3 Compare May 17, 2022 10:59
@andreasnoack
Copy link
Member

I think it would be better to put the checks in the functions that assume one-based indexing

function seed!(duals::AbstractArray{Dual{T,V,N}}, x,
seeds::NTuple{N,Partials{N,V}}) where {T,V,N}
dual_inds = 1:N
duals[dual_inds] .= Dual{T,V,N}.(view(x,dual_inds), seeds)
return duals
end
function seed!(duals::AbstractArray{Dual{T,V,N}}, x, index,
seed::Partials{N,V} = zero(Partials{N,V})) where {T,V,N}
offset = index - 1
dual_inds = (1:N) .+ offset
duals[dual_inds] .= Dual{T,V,N}.(view(x, dual_inds), Ref(seed))
return duals
end
function seed!(duals::AbstractArray{Dual{T,V,N}}, x, index,
seeds::NTuple{N,Partials{N,V}}, chunksize = N) where {T,V,N}
offset = index - 1
seed_inds = 1:chunksize
dual_inds = seed_inds .+ offset
duals[dual_inds] .= Dual{T,V,N}.(view(x, dual_inds), getindex.(Ref(seeds), seed_inds))
return duals
end
where the error you show is thrown from. Or maybe just change the indexing to be generic?

@mcabbott
Copy link
Member Author

mcabbott commented Aug 4, 2022

My hope is that eventually general indexing could be supported, and these checks can be removed as soon as there are tests that it works.

I didn't bother to figure out precisely where the failures were, but did check that all the functions to which this error was added do currently fail.

src/prelude.jl Outdated Show resolved Hide resolved
src/prelude.jl Outdated Show resolved Hide resolved
@mcabbott mcabbott merged commit 76335e6 into JuliaDiff:master Dec 13, 2022
@mcabbott mcabbott deleted the no_offsets branch December 13, 2022 14:51
devmotion pushed a commit that referenced this pull request Sep 30, 2024
* no offsets

* ... and no piracy

* import from Base
devmotion pushed a commit that referenced this pull request Sep 30, 2024
* no offsets

* ... and no piracy

* import from Base
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants