Propose some changes to DataFrames.jl API #68
Conversation
| @@ -13,7 +13,7 @@ Statistics = "10745b16-79ce-11e8-11f9-7d13ad32a3b2" | |||
| StatsBase = "2913bbd2-ae8a-5f71-8c99-4fb6c76f3a91" | |||
|
|
|||
| [compat] | |||
| DataFrames = "0.19, 0.20, 0.21, 0.22, 1" | |||
| DataFrames = "0.22, 1" | |||
| @@ -6,7 +6,7 @@ LearnBase.getobs(dt::AbstractDataFrame, idx) = dt[idx,:] | |||
|
|
|||
| LearnBase.nobs(dt::DataFrameRow) = 1 # it is a observation | |||
| function LearnBase.getobs(dt::DataFrameRow, idx) | |||
| idx == 1:1 || throw(ArgumentError( | |||
| only(idx) == 1 || throw(ArgumentError( | |||
There was a problem hiding this comment.
I understand that idx can be any collection as long as it has a single element equal to 1. Right?
| @@ -23,9 +23,12 @@ LearnBase.gettarget(::typeof(identity), dt::DataFrameRow) = | |||
| _throw_table_error() | |||
|
|
|||
| # convenience syntax to allow column name | |||
| LearnBase.gettarget(col::Symbol, dt::AbstractDataFrame) = dt[1, col] | |||
| LearnBase.gettarget(col::Symbol, dt::AbstractDataFrame) = | |||
There was a problem hiding this comment.
I understand that for this to work we should make sure that the data frame has only one row. Right?
Also the question is why do we only allow Symbol for col. Also integer or string index is allowed in DataFrames.jl.
| LearnBase.gettarget(col::Symbol, dt::DataFrameRow) = dt[col] | ||
| LearnBase.gettarget(fun, dt::AbstractDataFrame) = fun(dt) | ||
| LearnBase.gettarget(fun, dt::AbstractDataFrame) = |
There was a problem hiding this comment.
If I understand the intention correctly the fun form should also normally take a DataFrameRow as an argument. Right? This change is breaking (and I might have misunderstood the intention here).
|
CI is failing because |
|
Looks okay to me. Since I don't use DF very much, @oxinabox how does this look to you? |
|
Seems right to me though I haven't looked at LearnBase in literally years, and doing #46 seems like a better idea, but this is a net improvement. |
I will comment on the rationale of the changes inline (as I might not understand all the details of the intended design).