Fix handling of non-Int row indices #1177
Conversation
src/subdataframe/subdataframe.jl
Outdated
| @@ -71,7 +71,7 @@ function SubDataFrame(parent::DataFrame, row::Integer) | |||
| end | |||
|
|
|||
| function SubDataFrame{S <: Integer}(parent::DataFrame, rows::AbstractVector{S}) | |||
| return view(parent, Int(rows)) | |||
| return SubDataFrame{Vector{Int}}(parent, convert(Vector{Int}, rows)) | |||
There was a problem hiding this comment.
We should only convert to AbstractVector{Int} since that's what the type accepts. I suspect the Int(rows) expression comes from an automated replacement which was done after int(x) was deprecated. This was effectively equivalent to convert(AbstractVector{Int}, rows).
Also it seems it would more sense to remove this method and put this line into the generic constructor with rows::T above.
nalimilan
changed the title
|
Thanks. Note that |
|
Good points - I'll try to merge this into the earlier constructor and come back |
src/subdataframe/subdataframe.jl
Outdated
| @@ -62,18 +62,14 @@ immutable SubDataFrame{T <: AbstractVector{Int}} <: AbstractDataFrame | |||
| end | |||
| end | |||
|
|
|||
| function SubDataFrame{T <: AbstractVector{Int}}(parent::DataFrame, rows::T) | |||
| return SubDataFrame{T}(parent, rows) | |||
| function SubDataFrame{S <: Integer}(parent::DataFrame, rows::AbstractVector{S}) | |||
There was a problem hiding this comment.
Minor point, but using T is more common.
src/subdataframe/subdataframe.jl
Outdated
| function SubDataFrame{T <: AbstractVector{Int}}(parent::DataFrame, rows::T) | ||
| return SubDataFrame{T}(parent, rows) | ||
| function SubDataFrame{S <: Integer}(parent::DataFrame, rows::AbstractVector{S}) | ||
| return SubDataFrame{AbstractVector{Int}}(parent, convert(AbstractVector{Int}, rows)) |
There was a problem hiding this comment.
{AbstractVector{Int}} isn't correct here: it should be the actual concrete type to which rows was converted. So you need to store the result of the conversion in an object first, and call typeof on it.
There was a problem hiding this comment.
OK - thanks, that makes sense
Non-Int row-indices were not being correctly converted to Int.
- explicit conversion to Vector{Int}
- test-cases added
|
Thanks! |
|
This also happened with |
|
All my favourite Julia bugs seem to tie back to precompilation :) |
Non-
Int64row-indices were not being correctly converted toInt64.Vector{Int64}Interestingly, this came up only when DataFrames was precompiled into a userimg (see JuliaLang/julia#21164) and only after the rename of
sub()toview()