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

Use a consistent interface for Tables where input type cannot be converted #12

Closed
bencottier opened this issue Feb 4, 2021 · 3 comments
Labels

Comments

@bencottier
Copy link
Contributor

bencottier commented Feb 4, 2021

I'm uncertain about this. There's a more general question of what operations on Tables we want to provide, vs. leaving to the user.

Currently, we have an apply! for tables that mutates the table columns in-place.

However this doesn't work when the input type can't be converted to the output type, e.g. from DateTime to Int for the hour-of-day transform. We could replace the column with the new type, but users should have a choice whether to do such a "strong" mutation.

Alternatively we could have an apply! that appends the new columns to the Table.

We could also have an apply that returns the transformed columns on their own (example), which gives users the most flexibility.

@bencottier bencottier added the enhancement New feature or request label Feb 4, 2021
@bencottier bencottier mentioned this issue Feb 4, 2021
4 tasks
@bencottier bencottier changed the title Implement apply! that appends columns to Table Use a consistent interface for Tables where input type cannot be converted Feb 4, 2021
@nicoleepp
Copy link
Contributor

I don't like

Alternatively we could have an apply! that appends the new columns to the Table.

just because apply! done on Arrays would not have the same behaviour and that's confusing

For that reason I think

We could also have an apply that returns the transformed columns on their own (example), which gives users the most flexibility.

is the best option, and let the user append the result to the table explicitly

@nicoleepp
Copy link
Contributor

If we really wanted

Alternatively we could have an apply! that appends the new columns to the Table.

It should probably be a third function, like apply_append!(x::Table, ::Transfrom; newcolnames = [:a. :b], kwargs...)

or

apply!(x::Table, ::Transform; append=false, newcolnames=[:a, :b], kwargs...) but this last one might be confusing as sometimes it would mutate the cols that exist and sometimes append

@glennmoy
Copy link
Member

glennmoy commented Feb 4, 2021

is the best option, and let the user append the result to the table explicitly

It also gives them the flexibility to name it accordingly. Otherwise we'd have to come up with a naming convention in the package ☠️

It should probably be a third function, like apply_append!(x::Table, ::Transfrom; newcolnames = [:a. :b], kwargs...)

A slightly roundabout (but feasible) way is also

# for DataFrames
df.transformed = apply(df, ::Transform; kwargs...)

# for NamedTuples
x = apply(nt, ::Transform; kwargs...)
merge!(nt, (transformed=x,))

but if we had to make a single function call I'd prefer apply_append! as it's more explicit, can't be confused with normal apply! and doesn't rely on boolean kwarg.

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

No branches or pull requests

3 participants