Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add opt-in Tables.isrow trait (similar to istable)? #272

Open
jrevels opened this issue Jan 26, 2022 · 3 comments
Open

Add opt-in Tables.isrow trait (similar to istable)? #272

jrevels opened this issue Jan 26, 2022 · 3 comments

Comments

@jrevels
Copy link
Collaborator

jrevels commented Jan 26, 2022

Imagine DataFrames wanted to support push!(df::DataFrame, row) for arbitrary row as long as it's a valid Tables.jl row.

It'd be nice to enable DataFrames to implement this like:

push!(df::DataFrame, row) = Tables.isrow(row) ? #= do it =# : throw(ArgumentError("hey that's not a row!")

(could have Tables.isrow(::AbstractRow) = true defined by default)

This would let DataFrames (and other packages) support non-AbstractRow rows in a manner that is still "safe" for values that are indeed row-like but probably intended to be scalars in a tabular data setting, so better errors can be thrown (i.e. avoid throwing a weird getcolumn MethodError if push!(df, "hello") is called)

@bkamins
Copy link
Member

bkamins commented Jan 27, 2022

As a reference. Now, for safety reasons, in DataFrames.jl we only allow: Tuple, AbstractArray, AbstractDict, DataFrameRow and NamedTuple.

@rikhuijzer
Copy link

I also happen to stumble into this issue and also think that it could be nice. I think that it would be nice if Table.isrow(row::DataFrameRow) = true would be defined. This could help in StatsBase.predict, because that method now throws an ArgumentError for DataFrameRows. Instead, people have to manually construct a new DataFrame (or other Tables.istable object) from the DataFrameRow which is not very performant nor easy to use.

@bkamins
Copy link
Member

bkamins commented Jan 31, 2022

Currently for predict this is required because this is what the contract for predict assumes (of course we could change this):

An object with new covariate values newX can be supplied, which should have the same type and structure as that used to fit model;

(the problem here is that the contract does not require newX to be a table - it could be anything - of course normally it is a table)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants