setindex, push and convenience constructor #12

andreasnoack · 2018-01-10T14:43:27Z

I'm now sure if you want this package to be minimal or if you are okay with adding some functionality like this. If you are okay with adding stuff, I'll finish up the PR. I think this could be useful as an internal replacement for the now deprecated PooledDataArrays in Gadfly but then some extra functionality similar to the methods defined here are needed.

timholy · 2018-01-10T18:52:51Z

Interesting. On one hand I worry this violates some of the basic assumptions of AbstractArrays (getting and setting values should be fast), OTOH this does seem useful.

On balance I'm leaning in favor, but would be happy if other JuliaArrays contributors chimed in. Out of curiosity, how big is the values array in most of your use cases?

andreasnoack · 2018-01-10T19:09:41Z

On one hand I worry this violates some of the basic assumptions of AbstractArrays

When working with sparse and distributed arrays I learned to take the basic assumptions of AbstractArrays (for which setindex! is much more costly) with a grain of salt but I'm sympathetic to keeping things simple.

Out of curiosity, how big is the values array in most of your use cases?

My own use cases for these kinds of arrays have typically been statistics, i.e. what is called a factor in R and there the number of values has been very small. Typically smaller than ten. However, the current motivation was to bring Gadfly up to date which required a replacement for PooledDataArrays. When running the test suite using PooledArrays, I noticed that UInt32s were used which means that there must be quite a few values in that array since the default is to use UInt8.

timholy · 2018-01-10T19:34:40Z

I asked because I wondered if an inverse-mapping Dict might be better. But at less than 10 I suspect linear search would be hard to beat. We could also add another type that does this via a Dict (or static perfect-hashing algorithm), if the need arises.

Unless others object, I'm personally willing to take this. Thanks for stepping forward.

codecov-io · 2018-01-19T22:49:15Z

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          16     38   +22     
=====================================
+ Hits           16     38   +22

Impacted Files	Coverage Δ
src/IndirectArrays.jl	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 135a274...e32c8bb. Read the comment docs.

andreasnoack · 2018-02-15T20:51:57Z

I've added some tests so I think this is ready. Some of these methods can probably be optimized but for most applications, I only think a fairly fast getindex is required.

andreasnoack · 2018-02-19T07:24:16Z

@timholy Bump. This is blocking a Gadfly update so it would be great to get merged soon. Please let me know if it needs changes.

bjarthur · 2018-02-19T12:02:36Z

if folks object to this, am i correct in thinking that Gadfly could import and extend IndirectArrays instead?

andreasnoack · 2018-02-28T15:41:31Z

Bump

va-andrew · 2018-03-05T17:58:57Z

bump

nalimilan

Given that @timholy said he was OK on the principle, I'm going to (ab)use my powers and merge this unless somebody objects. It's been blocking the port of Gadfly to DataFrames 0.11 for too long.

nalimilan · 2018-03-09T21:44:40Z

src/IndirectArrays.jl

+@inline function Base.setindex!(A::IndirectArray, x, i::Int)
+    @boundscheck checkbounds(A.index, i)
+    idx = findfirst(A.values, x)
+    if idx == 0 || idx == nothing # findfird changed in 0.7


"findfirst". Could also use Compat.findfirst now.

Indeed. I've updated the PR.

nalimilan · 2018-03-09T22:18:40Z

src/IndirectArrays.jl

@@ -26,10 +28,19 @@ end
 Base.@propagate_inbounds IndirectArray(index::AbstractArray{<:Integer,N}, values::AbstractVector{T}) where {T,N} =
    IndirectArray{T,N,typeof(index),typeof(values)}(index, values)

+function (::Type{IndirectArray{T}})(A::AbstractArray, values::AbstractVector = unique(A)) where {T}
+    # Use map! to make sure that index is an Array
+    index = map!(t -> findfirst(values, t), Array{T}(size(A)...), A)


Sorry, I missed this. Much better to use indexin(A, values) here AFAICT. Not sure what should happen if some values of A do not appear in values: is throwing an error OK?

For maximal performance, it would even make sense to use values = nothing by default, and compute unique values using the same dict as the one created by indexin.

nalimilan · 2018-03-11T21:56:24Z

I've tagged a release: JuliaLang/METADATA.jl#13782

Add setindex!, push!, and append!

57caa8a

Mattriks mentioned this pull request Jan 12, 2018

[WIP] work towards updating to DataFrames v0.11+ framework GiovineItalia/Gadfly.jl#1088

Closed

Add convenience constructors

6ad8e19

andreasnoack force-pushed the anj/setindex branch from 89fe7f3 to 6ad8e19 Compare January 19, 2018 22:49

Remove disambiguation method since the ambiguity is spurious

a07e155

andreasnoack mentioned this pull request Jan 22, 2018

DataFrames 0.11+ compat via IndirectArrays GiovineItalia/Gadfly.jl#1090

Merged

andreasnoack force-pushed the anj/setindex branch from b86df94 to 66906c6 Compare February 15, 2018 20:28

Add tests

b637a1b

andreasnoack force-pushed the anj/setindex branch from 66906c6 to b637a1b Compare February 15, 2018 21:44

findfirst behavior has changed on 0.7

ffecc67

nalimilan reviewed Mar 9, 2018

View reviewed changes

Use Compat.findfirst

fc28287

nalimilan reviewed Mar 9, 2018

View reviewed changes

andreasnoack and others added 2 commits March 11, 2018 22:36

Use indexin instead of findfirst with map

fe2e6c6

Remove outdated mention of map!

e32c8bb

nalimilan merged commit b915ef0 into JuliaArrays:master Mar 11, 2018

andreasnoack deleted the anj/setindex branch March 11, 2018 22:01

Marlin-Na mentioned this pull request Dec 13, 2020

Correct README: setindex! is supported #22

Merged

timholy mentioned this pull request Apr 14, 2023

A hidden bug caused by indirect array (possibly) JuliaIO/PNGFiles.jl#63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

setindex, push and convenience constructor #12

setindex, push and convenience constructor #12

andreasnoack commented Jan 10, 2018 •

edited

Loading

timholy commented Jan 10, 2018

andreasnoack commented Jan 10, 2018 •

edited

Loading

timholy commented Jan 10, 2018

codecov-io commented Jan 19, 2018 •

edited

Loading

andreasnoack commented Feb 15, 2018

andreasnoack commented Feb 19, 2018

bjarthur commented Feb 19, 2018

andreasnoack commented Feb 28, 2018

va-andrew commented Mar 5, 2018

nalimilan left a comment

nalimilan Mar 9, 2018

andreasnoack Mar 9, 2018

nalimilan Mar 9, 2018

nalimilan commented Mar 11, 2018

setindex, push and convenience constructor #12

setindex, push and convenience constructor #12

Conversation

andreasnoack commented Jan 10, 2018 • edited Loading

timholy commented Jan 10, 2018

andreasnoack commented Jan 10, 2018 • edited Loading

timholy commented Jan 10, 2018

codecov-io commented Jan 19, 2018 • edited Loading

Codecov Report

andreasnoack commented Feb 15, 2018

andreasnoack commented Feb 19, 2018

bjarthur commented Feb 19, 2018

andreasnoack commented Feb 28, 2018

va-andrew commented Mar 5, 2018

nalimilan left a comment

Choose a reason for hiding this comment

nalimilan Mar 9, 2018

Choose a reason for hiding this comment

andreasnoack Mar 9, 2018

Choose a reason for hiding this comment

nalimilan Mar 9, 2018

Choose a reason for hiding this comment

nalimilan commented Mar 11, 2018

andreasnoack commented Jan 10, 2018 •

edited

Loading

andreasnoack commented Jan 10, 2018 •

edited

Loading

codecov-io commented Jan 19, 2018 •

edited

Loading