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

Fix assignment to 0-dim array indexed by CartesianIndices{0} #34893

Merged
merged 2 commits into from
Feb 28, 2020

Conversation

jmert
Copy link
Contributor

@jmert jmert commented Feb 26, 2020

The change in PR #31214 replaced CartesianIndices in the indexing tuple with their constituent ranges. This broke assignment to 0-dimensional arrays which have been indexed by a CartesianIndices{0}, e.g.

julia> A = fill(0.0);

julia> I = CartesianIndices(size(A));

julia> x = fill!(similar(A), 1.0);

# On v1.3
julia> A[I] = x
0-dimensional Array{Float64,0}:
1.0

# On master
julia> A[I] = x
ERROR: MethodError: Cannot `convert` an object of type Array{Float64,0} to an object of type Float64
...

This is because previously the CartesianIndices{0} (hereafter CI0) was preserved through the call to to_indices():

# On v1.3
julia> to_indices(A, (I,))
(CartesianIndex(),)

julia> to_indices(A, (I,I))
(CartesianIndex(), CartesianIndex())

julia> to_indices(A, (I,I,I))
(CartesianIndex(), CartesianIndex(), CartesianIndex())

but now the empty tuple is returned instead:

# On master
julia> to_indices(A, (I,))
()

julia> to_indices(A, (I,I))
()

julia> to_indices(A, (I,I,I))
()

and that changes which internal implementation of setindex! is dispatched to.

This PR fixes the issue by tweaking the tuple reduction performed by to_indices(). The result is that
during the tuple reduction, a case where only [trailing] CI0s remain is shortcut and returns just a single CI0 index.

With this PR:

julia> A[I] = x
0-dimensional Array{Float64,0}:
1.0

julia> to_indices(A, (I,))
(fill(CartesianIndex()),)

julia> to_indices(A, (I,I))
(fill(CartesianIndex()),)

julia> to_indices(A, (I,I,I))
(fill(CartesianIndex()),)

The one thing I couldn't figure out is how to avoid an unnecessary trailing CI0 if a concrete index precedes it — (my attempts lead to to_indices() method ambiguities) — but there's always at most a single unnecessary trailing CI0.

julia> B = [1.0]
1-element Array{Float64,1}:
 1.0

julia> to_indices(B, (1, I))
(1, fill(CartesianIndex()))

julia> to_indices(B, (1, I, I))
(1, fill(CartesianIndex()))

julia> to_indices(B, (I, 1, I))
(1, fill(CartesianIndex()))

julia> to_indices(B, (I, I, 1, I))
(1, fill(CartesianIndex()))

julia> to_indices(B, (I, I, I, 1))
(1,)

@dkarrasch dkarrasch requested a review from mbauman February 26, 2020 21:11
@jmert
Copy link
Contributor Author

jmert commented Feb 28, 2020

I found another case that has regressed, and this PR right now doesn't fix.

julia> I = CartesianIndices(())
0-dimensional CartesianIndices{0,Tuple{}}:
CartesianIndex()

# On v1.3
julia> setindex!(zeros(2), fill(1.0), I, 1)
2-element Array{Float64,1}:
 1.0
 0.0

# On master
julia> setindex!(zeros(2), fill(1.0), I, 1)
ERROR: MethodError: Cannot `convert` an object of type Array{Float64,0} to an object of type Float64
...

@jmert jmert force-pushed the setindex_cartesianindex0 branch from 8283e7f to 0de6f2a Compare February 28, 2020 03:37
@jmert
Copy link
Contributor Author

jmert commented Feb 28, 2020

It appears simply preserving all instances of CartesianIndices(()) in the indexing tuple fixes the case in my previous comment. I've added a few new tests for that as well.

@mbauman
Copy link
Member

mbauman commented Feb 28, 2020

Ah, very nice, thank you! There's an ambiguity when there are no indices — was this a scalar assignment or a non scalar assignment? #24086 is the root cause here, but this is a great fix. Thanks!

@mbauman mbauman added backport 1.4 bugfix This change fixes an existing bug labels Feb 28, 2020
@jmert
Copy link
Contributor Author

jmert commented Feb 28, 2020

Correct me if I'm wrong, but I don't think this PR needs to be backported since #31214 was only merged after the v1.4 branch (assuming I'm reading the tag/branch "breadcrumb" in Github's view of f579d14 correctly).

@mbauman
Copy link
Member

mbauman commented Feb 28, 2020

Right you are. Thanks again!

@mbauman mbauman merged commit 6dd9f41 into JuliaLang:master Feb 28, 2020
@jmert jmert deleted the setindex_cartesianindex0 branch February 28, 2020 22:52
ravibitsgoa pushed a commit to ravibitsgoa/julia that referenced this pull request Apr 9, 2020
…ng#34893)

* Fix assignment to 0-dim array indexed by CartesianIndices{0}

* Fix assigning element into ≥1-dim arrays from 0-dim array
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
* Fix assignment to 0-dim array indexed by CartesianIndices{0}

* Fix assigning element into ≥1-dim arrays from 0-dim array
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants