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

copyto! breaking change in v1.5 #36220

Closed
goretkin opened this issue Jun 10, 2020 · 5 comments · Fixed by #36397
Closed

copyto! breaking change in v1.5 #36220

goretkin opened this issue Jun 10, 2020 · 5 comments · Fixed by #36397
Labels
arrays [a, r, r, a, y, s]
Milestone

Comments

@goretkin
Copy link
Contributor

goretkin commented Jun 10, 2020

using OffsetArrays

oa = fill(1, (3:5, 3:5))
copyto!(Array{Int,2}(undef, size(oa)), oa)

On v1.4:

ERROR: BoundsError: attempt to access 3×3 Array{Int64,2} at index [3:5, 3:5]
Stacktrace:
 [1] throw_boundserror(::Array{Int64,2}, ::Tuple{OffsetArrays.IdOffsetRange{Int64,Base.OneTo{Int64}},OffsetArrays.IdOffsetRange{Int64,Base.OneTo{Int64}}}) at ./abstractarray.jl:537
 [2] checkbounds at ./abstractarray.jl:502 [inlined]
 [3] copyto!(::Array{Int64,2}, ::OffsetArray{Int64,2,Array{Int64,2}}) at ./multidimensional.jl:959
 [4] top-level scope at none:0

via a dispatch to

function copyto!(dest::AbstractArray{T1,N}, src::AbstractArray{T2,N}) where {T1,T2,N}
checkbounds(dest, axes(src)...)
src′ = unalias(dest, src)
for I in eachindex(IndexStyle(src′,dest), src′)
@inbounds dest[I] = src′[I]
end
dest
end

whereas on v1.5 it does not throw.

f345c5f may be the cause.

goretkin referenced this issue Jun 10, 2020
When the eltype and/or dimensionality is unspecified, there were
2 or 3 methods that might apply here (see
`code_typed(copyto!, (Array, Array))`). Consequently partially-specified
codegen tends to be poor for this operation.

This harmonizes our implementations and eliminates eltype and dimensionality
as a consideration for the dispatch. By default, it dispatches on
`IndexStyle(src)` after a call to `unalias`, which makes sense because
copying an array can change its preferred `IndexStyle`.
It also fixes an aliasing bug.
@rfourquet rfourquet added the arrays [a, r, r, a, y, s] label Jun 10, 2020
@StefanKarpinski StefanKarpinski added this to the 1.5 milestone Jun 10, 2020
@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Jun 10, 2020
@goretkin
Copy link
Contributor Author

I don't have such a strong opinion that the copyto! behavior be restored. I do think the following is very bad:

julia> VERSION
v"1.5.0-beta1.0"

julia> using OffsetArrays

julia> r = Ref(fill(1, (Base.OneTo(3), Base.OneTo(3))))
Base.RefValue{Array{Int64,2}}([1 1 1; 1 1 1; 1 1 1])

julia> r[] = fill(2, (3:5, 3:5))
3×3 OffsetArray(::Array{Int64,2}, 3:5, 3:5) with eltype Int64 with indices 3:5×3:5:
 2  2  2
 2  2  2
 2  2  2

julia> r[]
3×3 Array{Int64,2}:
 2  2  2
 2  2  2
 2  2  2

@timholy
Copy link
Member

timholy commented Jun 12, 2020

The new behavior is correct according to copyto!'s docstring, which is typically the authoritative source. I haven't dug into the other issue, but it indeed looks like a bug.

@JeffBezanson
Copy link
Member

From triage: this is ok for copyto!, but not for convert, since an OffsetArray is not == to the same data inside an Array. We need to add a check to convert.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jun 18, 2020
@KristofferC
Copy link
Member

Bump, anyone looking at this?

@timholy
Copy link
Member

timholy commented Jun 23, 2020

The hard question is whether Array(OA) should throw an error or just convert(Array, OA). I favor throwing an error for both, but it is not obvious.

KristofferC pushed a commit that referenced this issue Jun 25, 2020
mbauman added a commit to dlfivefifty/julia that referenced this issue Jun 26, 2020
* origin/master: (232 commits)
  Add passthrough for non-Markdown docs (JuliaLang#36091)
  Fix pointer to no longer assume contiguity (JuliaLang#36405)
  Ensure string-hashing is defined before it gets used (JuliaLang#36411)
  Make compilecache atomic (JuliaLang#36416)
  add a test for JuliaLang#30739 (JuliaLang#36395)
  Fix broken links in docstring of `repeat` (JuliaLang#36376)
  fix and de-dup cached calls to `methods_by_ftype` in compiler (JuliaLang#36404)
  ml-matches: skip unnecessary work, when possible (JuliaLang#36413)
  gf: fix some issues with the move from using a tree to a hash lookup of leaf types (JuliaLang#36413)
  Add news and manual entry for sincospi (JuliaLang#36403)
  Check axes in Array(::AbstractArray) (fixes JuliaLang#36220) (JuliaLang#36397)
  add versions of `code_typed` and `which` that accept tuple types (JuliaLang#36389)
  Fix spelling of readdir. (JuliaLang#36409)
  add sincospi (JuliaLang#35816)
  fix showing methods with unicode gensymed variable names (JuliaLang#36396)
  Add doctest: eachslice (JuliaLang#36386)
  fix documentation typo ("Ingeger")
  Refactor `abstract_eval` to separate out statements and values (JuliaLang#36350)
  fix return type of `get!` on `IdDict` (JuliaLang#36383)
  Allow single option with REPL.TerminalMenus (JuliaLang#36369)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants