RFC: setindex for mutable collections #33495

tkf · 2019-10-08T04:49:53Z

setindex was documented recently #32738 and there are also some interests in defining it for general arrays (e.g., ArrayInterface.jl, Setfield.jl).

I propose the following definition of setindex for (possibly) mutable collections. I believe this is more useful than more straightforward implementation like setindex!(copy(collection), value, key...). This is because it can be used as a building block of the "mutate-or-widen" strategy that is used in functions like collect and materialize.

I'm using this definition in BangBang.jl and it turned out to be quite useful for writing efficient collect etc. for Transducers.jl while avoiding depending on the compiler's inference API.

Proposed API

setindex(collection, value, key...)
Create a new collection with the element/elements of the location specified by key
replaced with value(s).

Implementation

setindex(collection, value, key...) must have the property such that
y1 = setindex(x, value, key...)

y2 = deepcopy(x)
setindex!(y2, value, key...)

@assert isequal′(y1, y2)
with a suitable definition of isequal′ (e.g., elements are approximately but possibly
not exactly equal), if setindex! supports given arguments.

setindex should support more combinations of arguments by widening collection
type as required.

I also included the implementations for arrays and dicts.

jw3126 · 2019-12-10T07:46:14Z

I think this PR is quite useful for writing generic code that works on both immutable and mutable collections. To me this looks is a basic interface, that really should live in Base.

mbauman

This is great! I'm in full support of adding this functionality. It still doesn't fully complete the copy-and-widen story, but it's a huge helping block in getting there.

mbauman · 2019-12-10T21:44:43Z

base/array.jl

+```
+
+with a suitable definition of `isequal′` (e.g., elements are approximately but possibly
+not exactly equal), _if_ `setindex!` supports given arguments.


The not exactly equal part here is just because we have, e.g., convert(Float64, Int(maxintfloat()) + 1) != Int(maxintfloat()) + 1, right? Maybe it'd be a bit clearer to directly say that it's equal after conversion? I think I'd rather focus on the copy side of things than the equality side of things:

That is, something more like:

y1 = setindex(x, value, key...) y2 = copy′(x) y2[key...] = value @assert convert.(eltype(y2), y1) == y2

with a suitable definition of copy′ that ensures value can be assigned into the collection. This is where the focus should be. Getting the @assert exactly right (which it's not right now because broadcast) is slightly less important than getting the widening right, I think.

That way we don't have to bend over backwards with awkward approximately-but-maybe-not-exact sorts of phrasings. It's just a (possible convert plus) copy plus setindex!.

This is a great idea! Yes, it was a very awkward part of my version of the specification. Your version is much more elegant and precise.

mbauman · 2019-12-10T21:52:40Z

base/array.jl

+function setindex(xs::AbstractArray, v, I...)
+    T = promote_type(eltype(xs), typeof(v))
+    ys = similar(xs, T)
+    if eltype(xs) !== Union{}


In what cases will promote_type return Union{}? And why can't we copy! a Union{} array?

julia> copy!(Array{Union{}}(undef, 0), Array{Union{}}(undef, 0)) 0-element Array{Union{},1}

It is sometimes useful to specify the container type and shape without specifying its element type (e.g., for implementing multi-dim map in the mutate-or-widen style). Union{} element type is a nice way to encode this. But you can't copy! from it.

julia> copy!(Vector{Int}(undef, 3), Vector{Union{}}(undef, 3)) ERROR: UndefRefError: access to undefined reference Stacktrace: [1] getindex(::Array{Union{},1}, ::Int64) at ./array.jl:758 [2] copyto!(::Array{Int64,1}, ::Int64, ::Array{Union{},1}, ::Int64, ::Int64) at ./abstractarray.jl:842 [3] append! at ./array.jl:926 [inlined] [4] copy!(::Array{Int64,1}, ::Array{Union{},1}) at ./abstractarray.jl:709 [5] top-level scope at REPL[5]:1

This error itself is not specific to Union{}; i.e., copy! does not handle undef values. eltype(xs) !== Union{} is just an easy shortcut to handle a useful case. But maybe this logic should be moved to copy! function?

I just removed this branching for now. Adding this to copy! (in a different PR) seems to be the right direction.

tkf · 2019-12-10T23:00:24Z

base/array.jl

+function setindex end
+
+function setindex(xs::AbstractArray, v, I...)
+    T = promote_type(eltype(xs), typeof(v))


Should we use promote_typejoin instead?

ATM, eltype(setindex([0, 0], 0.5, 1)) is Float64. It'd be Real if we use promote_typejoin. The former is compatible with how vcat work and the latter is compatible with how collect and broadcast work.

I'd say promote_type is the right choice. Otherwise you'd very often get abstract element types, which really kill performance.

jw3126 · 2019-12-11T08:59:48Z

test/arrayops.jl


+@testset "setindex" begin
+    ==ₜ(_, _) = false
+    ==ₜ(x::T, y::T) where T = x == y


==ₜ seems so handy, maybe it should actually live in e.g. Test?

test/arrayops.jl

base/array.jl

nalimilan · 2019-12-12T08:17:27Z

base/array.jl

+    setindex(collection, value, key...)
+
+Create a new collection with the element/elements of the location specified by `key`
+replaced with `value`(s).


Also mention that the new collection may have a different element type, as determined by promote_type.

nalimilan · 2019-12-12T08:18:30Z

base/array.jl

+function setindex end
+
+function setindex(xs::AbstractArray, v, I...)
+    T = promote_type(eltype(xs), typeof(v))


I'd say promote_type is the right choice. Otherwise you'd very often get abstract element types, which really kill performance.

base/dict.jl

nalimilan · 2019-12-12T08:21:51Z

test/arrayops.jl

+    ==ₜ(_, _) = false
+    ==ₜ(x::T, y::T) where T = x == y
+
+    @test Base.setindex(Int[1, 2], 3.0, 2) ==ₜ [1.0, 3.0]


Suggested change

@test Base.setindex(Int[1, 2], 3.0, 2) ==ₜ [1.0, 3.0]

@test Base.setindex([1, 2], 3.0, 2) ==ₜ [1.0, 3.0]

I think it is good to have Int explicit, since the test is about change of eltype.

What's the point since that doesn't make any difference?

nalimilan · 2019-12-12T08:21:59Z

test/arrayops.jl

+    ==ₜ(x::T, y::T) where T = x == y
+
+    @test Base.setindex(Int[1, 2], 3.0, 2) ==ₜ [1.0, 3.0]
+    @test Base.setindex(Int[1, 2, 3], :two, 2) ==ₜ [1, :two, 3]


Suggested change

@test Base.setindex(Int[1, 2, 3], :two, 2) ==ₜ [1, :two, 3]

@test Base.setindex([1, 2, 3], :two, 2) ==ₜ [1, :two, 3]

jw3126 · 2021-04-23T10:33:19Z

I would like to have this functionality in Base. @tkf I guess you don't have much time for this. Would it be okay for you if I take this PR and rebase/finish it?

MasonProtter · 2023-05-09T22:42:46Z

@jw3126 Are you still interested in doing that? If so, I heartily encourage you to do it!

jw3126 · 2023-05-10T06:12:19Z

@MasonProtter I would love to have that. There is also #46453 and I think there is also at least one additional similar PR somewhere.
I am willing to try yet another PR, but only if somebody (ideally with merge rights) promises to do the required lobby work so it gets either merged or rejected for good technical reasons.

vchuravy · 2023-08-27T21:40:56Z

I am going to close this in favor of #46453 (and me sneaking in parts of it in #51066)

Implement setindex for AbstractArray and Dict

b64dc59

This was referenced Oct 10, 2019

Define functional setters using Setfield queryverse/VegaLite.jl#191

Merged

add setindex for named tuples #33468

Merged

jw3126 mentioned this pull request Dec 10, 2019

[RFC] monkey patch setindex jw3126/Setfield.jl#107

Merged

Merge branch 'master' into setindex

742cc7c

mbauman reviewed Dec 10, 2019

View reviewed changes

tkf added 2 commits December 10, 2019 14:49

Improve setindex specification

4743df1

Remove special handling of eltype(xs) !== Union{}

9f9065a

tkf commented Dec 10, 2019

View reviewed changes

jw3126 reviewed Dec 11, 2019

View reviewed changes

test/arrayops.jl Show resolved Hide resolved

tkf added 2 commits December 11, 2019 01:51

Test that setindex does not mutate input

b204b6c

Test inferrability of setindex

b57088b

nalimilan reviewed Dec 12, 2019

View reviewed changes

tkf mentioned this pull request Dec 14, 2019

Optimize copy!(::Dict, ::Dict) #34101

Merged

tkf mentioned this pull request Jan 22, 2020

Standard API for push_widen! pattern #34478

Open

tkf mentioned this pull request Feb 18, 2020

Consider making push/pushfirst/insert widen the type JuliaArrays/StaticArrays.jl#732

Open

tkf added the collections Data structures holding multiple items, e.g. sets label Jun 2, 2020

jw3126 mentioned this pull request Feb 11, 2021

Setting OffsetArray(::StaticArray) falling back to Vector jw3126/Setfield.jl#153

Open

jw3126 mentioned this pull request Aug 1, 2021

setindex fails on arrays with boxed undef jw3126/Setfield.jl#157

Open

jw3126 mentioned this pull request Oct 24, 2021

Drop optional dependency jw3126/Setfield.jl#164

Open

mateuszbaran mentioned this pull request Jun 7, 2022

Remove unnecessary methods from setindex JuliaArrays/ArrayInterface.jl#305

Merged

Tokazama mentioned this pull request Aug 23, 2022

Functional non-mutating methods. #46453

Open

vchuravy closed this Aug 27, 2023

jw3126 mentioned this pull request Feb 16, 2024

Add @check macro for non-disable-able @assert #41342

Open

aplavin mentioned this pull request Jul 24, 2024

make Base.setindex() public #55129

Open

	@test Base.setindex(Int[1, 2], 3.0, 2) ==ₜ [1.0, 3.0]
	@test Base.setindex([1, 2], 3.0, 2) ==ₜ [1.0, 3.0]

	@test Base.setindex(Int[1, 2, 3], :two, 2) ==ₜ [1, :two, 3]
	@test Base.setindex([1, 2, 3], :two, 2) ==ₜ [1, :two, 3]

Uh oh!

RFC: setindex for mutable collections #33495

RFC: setindex for mutable collections #33495

Uh oh!

Conversation

tkf commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed API

Uh oh!

jw3126 commented Dec 10, 2019

Uh oh!

mbauman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jw3126 commented Apr 23, 2021

Uh oh!

MasonProtter commented May 9, 2023

Uh oh!

jw3126 commented May 10, 2023

Uh oh!

vchuravy commented Aug 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tkf commented Oct 8, 2019 •

edited

Loading