JuliaData / DataFrames.jl

In-memory tabular data in Julia
https://dataframes.juliadata.org/stable/
Other
1.71k stars 360 forks source link

Cannot perform 2D DataArray assignment #935

Closed gear closed 7 years ago

gear commented 8 years ago

Hi,

I might be missing something here, but I think it's a bug. This is my test code:

deye = @data(eye(10))
deye[1,1] = NA
# ERROR: MethodError: no method matching unsafe_getindex(::Int64, ::Int64)
# Closest candidates are:
# unsafe_getindex(::AbstractArray{T,N}, ::Any...)
# unsafe_getindex(::Colon, ::Any)
# [inlined code] from ./cartesian.jl:60
# in setindex!(::DataArrays.DataArray{Float64,2}, ::DataArrays.NAtype, ::Int64, ::Int64) at
# /home/hoangnt/.julia/v0.5/Compat/src/ngenerate.jl:56
# in eval(::Module, ::Any) at ./boot.jl:243
nalimilan commented 8 years ago

Confirmed on 0.5, but it works on 0.4. This is a DataArrays issue, not a DataFrames one.

nalimilan commented 8 years ago

Note that deye[1,1] = 1.0 also fails. The bug does not happen in the 1D case.

The problematic code (missing from the stacktrace) is here: https://github.com/JuliaStats/DataArrays.jl/blob/44192cf6261d6eed476018fb2770c5ca8dc4e9f2/src/indexing.jl#L190

Looks like simply adding this definition is enough to fix the problem:

Base.unsafe_getindex(x::Int, i::Int) = x[i]

@mbauman Any reason why this method isn't present in 0.5? Base.unsafe_getindex(1, 1) works in 0.4.5.

mbauman commented 8 years ago

It's just no longer needed, replaced by @inbounds 42[1]. I knew it was frequently used, though, so I did keep a fallback for AbstractArray… but of course that doesn't cover numbers.

gear commented 8 years ago

Thank you for the answer. Sorry, I should put this in DataArrays.

nalimilan commented 8 years ago

@malmaud But will it be fast on 0.4 too?

Also, if a fallback is present for some types, it should probably be added for all of them, or removed altogether.

mbauman commented 8 years ago

Yeah, I didn't mean to break unsafe_getindex since it's pretty widely used (even if not exported or documented). And, no, @inbounds won't remove that branch on 0.4. That method can get restored.

nalimilan commented 7 years ago

Closing now that we no longer use DataArrays.