Fill existing arrays with scalars #1057
Conversation
TotalVerb
force-pushed
the
fw/fix-scalar-again
branch
from
e6b4a50 to
98d0303
Compare
| @@ -368,7 +380,7 @@ end | |||
| function Base.setindex!(df::DataFrame, | |||
| v::Any, | |||
| col_ind::ColumnIndex) | |||
| insert_single_column!(df, upgrade_scalar(df, v), col_ind) | |||
| fill_single_column!(df, v, col_ind) | |||
There was a problem hiding this comment.
Couldn't setindex! for the column type handle this, e.g. something like
df[:col_ind] = v?
There was a problem hiding this comment.
This is setindex! for the column type.
There was a problem hiding this comment.
But you're right in that the fill_single_column! function is just setindex!. I suppose it would be easier just to inline this function.
There was a problem hiding this comment.
Sorry. What I meant was to use setindex for the DataVector/NullableVector which would probably be something like
df[:col_ind][:] = vThere was a problem hiding this comment.
Good point. Unfortunately df[:col_ind] .= v does not work (even on v0.6) due to broadcast! being a little particular with what data types it considers "vectors", such as String. Also, I recall there was a discussion about setindex!(A, v, Colon()) still being slower than fill!(A, v), though I don't know if this issue has been addressed by now.
There was a problem hiding this comment.
It looks like JuliaLang/julia#16310 has closed the performance difference, although that means it still exists on v0.4. I think keeping fill! might be better for now.
There was a problem hiding this comment.
I've inlined the fill_single_column! function now into setindex!.
TotalVerb
force-pushed
the
fw/fix-scalar-again
branch
from
98d0303 to
60fd1c2
Compare
|
Great. Seems much simpler. Don't the tests fit into |
|
There used to be a few more tests for vector |
TotalVerb
force-pushed
the
fw/fix-scalar-again
branch
from
60fd1c2 to
e1122c3
Compare
|
OK, test merged into index.jl now. |
| function Base.setindex!(df::DataFrame, v, col_ind::ColumnIndex) | ||
| if haskey(index(df), col_ind) | ||
| j = index(df)[col_ind] | ||
| fill!(df.columns[j], v) |
There was a problem hiding this comment.
Doesn't fill!(df[col_ind], v) work here?
TotalVerb
force-pushed
the
fw/fix-scalar-again
branch
from
e1122c3 to
b543f43
Compare
TotalVerb
force-pushed
the
fw/fix-scalar-again
branch
from
b543f43 to
1d6d184
Compare
|
Is this ready to be merged now? |
(cherry picked from commit 2931693)
Closes #1053 (replacing it). Fixes #1051. Closes (maybe?) #1054, pending possible further discussion.
This implements @nalimilan and @andreasnoack's idea of filling arrays with scalars instead of creating new ones.