add rename macro, docs, and tests #343

MatthewRGonzalez · 2022-11-03T23:20:18Z

This pull request resolves #204 by providing new functionalities to DataFramesMeta.jl

New Macros

The following macros are added

@rename and @rename!

These macros extend the functionality of DataFrames.rename and DataFrames.rename! to the DataFramesMeta.jl ecosystem. These macros also allow arbitrary RHS expressions (per #204 (comment)).

Tests

General tests are added in rename.jl
Tests pass locally in runtests.jl

For convenience, I added a simple helper function that converts pair elements of the final argument transformation to strings. I'm happy to make any modifications. Thanks!

bkamins · 2022-11-04T08:05:03Z

src/macros.jl

+* `::AbstractDataFrame`
+
+Inputs to `@rename` can come in two formats: a `begin ... end` block, or as a series of
+arguments and keyword-like arguments. For example, the following are equivalent:


it is not clear for me in the docstring what "arguments" means and what "keyword-like arguments" means. In particular if we have a form lhs = rhs what values are accepted on both sides.

it would be also good too mention that whole expressions can be escaped by $ (like in the tests).

Hi, @bkamins. Thanks for the review! I've updated the docs to clarify phrasing regarding the points you raised above. I'm happy to continue revising as you see fit. Thanks!

Looks good. Thank you!

pdeffebach · 2022-11-08T19:34:53Z

Thanks for your work on this!

I think we need to re-think how it's done, however.

Currently, the whole RHS of the expression is passed to the whole fun_to_vec machinery. That is, for complicated expressions, we make anonymous functions and pass those to the renaming function.

julia> MacroTools.@macroexpand(@rename df :new = :old_col1 * :old_col3) |> MacroTools.prettify
:(make_renamed(df, DataFramesMeta.make_source_concrete([:old_col1, :old_col3]) => ((*) => :new)))

julia> @rename df :new = :old_col1 * :old_col3
ERROR: ArgumentError: Tried renaming :[:old_col1, :old_col3] to :(*) => :new, when :[:old_col1, :old_col3] does not exist in the data frame.

This seems like it could be a source of bugs.

That said, I like how we restrict the RHS to be a "valid" column identifier. I think it's good to be consistent about differences between columns and data, even if some users might find it annoying.

Can you try writing a helper function instead of fun_to_vec which just checks that the LHS and the RHS are both column identifiers and then makes a pair?

MatthewRGonzalez · 2022-11-09T18:28:26Z

Can you try writing a helper function instead of fun_to_vec which just checks that the LHS and the RHS are both column identifiers and then makes a pair?

Sure! I added a helper function called fun_to_pair in parsing.jl. This function is a simpler version of fun_to_vec that relies on get_column_expr() to determine the validity of column identifiers. Please let me know if this is in line with you're expectations. Thanks!

pdeffebach

Thanks!

pdeffebach · 2022-11-15T19:44:48Z

src/parsing.jl

@@ -387,6 +387,54 @@ fun_to_vec(ex::QuoteNode;
           gensym_names::Bool=false,
           outer_flags::Union{NamedTuple, Nothing}=nothing) = ex

+
+"""
+    fun_to_pair(ex::Expr)


Can you change the name of this function? it's not really fun_to_pair, more like rename_kw_to_pair or something.

pdeffebach · 2022-11-15T19:46:31Z

src/parsing.jl

+
+    ex_col = get_column_expr(ex)        
+    if ex_col !== nothing         
+        str_p = MacroTools.postwalk(x->x isa QuoteNode ? String(x.value) : x, ex_col )                          


This is very fragile. For example it will fail on

$(x => y)

where x and y are both QuoteNodes.

I propose to just check if an expression is of the form a = b and if it's not, then just return that expression unaltered. We will deal with any inconsistencies at runtime.

This is what we do in fun_to_vec with the lines

ex_col = get_column_expr(ex) if ex_col !== nothing return ex_col end

pdeffebach · 2022-11-15T19:49:07Z

src/parsing.jl

+
+    lhs = let t = ex.args[1]
+        if t isa Symbol
+            t = QuoteNode(t)


Don't handle this case. Just throw an error. This is a new function and we don't want old behavior to live forever.

See what error is thrown if you omit this check. If it's an okay error, that's good. If the error is something really obscure and hard for users to understand, add an explicit error message.

pdeffebach · 2022-11-15T19:52:11Z

src/parsing.jl

+        s
+    end
+
+    rhs = MacroTools.unblock(ex.args[2])


Do we really need the unblock here? If you want to do complicated things, you would still need to hide things behind $(...).

The unblock allows for the use of begin ... end in cases such as the following:

@rename df :newcol = begin $("old_col" * "1") end

Wouldn't you need a $ in there though? Since we don't allow arbitrary expressions on the RHS that are not wrapped in $(). So this seems redundant.

You're right. I edited the above to include $. If I remove the unblock as in the following:

rhs = ex.args[2] rhs_col = get_column_expr(rhs) if rhs_col === nothing throw(ArgumentError("Invalid column identifier on RHS in DataFramesMeta.jl macro")) end # parsing.jl:424

then

@rename df :newcol = begin $("old_col" * "1") end

throws Invalid column identifier on RHS in DataFramesMeta.jl macro. I'll look this over more.

I think its okay to disallow the second case. People can do

$( begin end)``` if they want to.

pdeffebach · 2022-11-15T20:13:42Z

src/macros.jl

+    x, exprs, outer_flags, kw = get_df_args_kwargs(x, args...; wrap_byrow = false)    
+    t = (fun_to_pair(ex) for ex in exprs)            
+    quote            
+        $DataFrames.rename!($x, $(t...))


Extended from comments below. Instead of doing String(lhs) => String(rhs) in the parsing stage, let's clean up the inputs all together.

I would allocate an intermediate array of renaming calls. then write a function that turns every vector of pairs into a vector of pairs of strings. Then pass that to rename. Something like

julia> function pairs_to_str_pairs(args...) map(args) do arg if !(arg isa Pair) throw(ArgumentError("Non-pair created in @rename")) end string(first(arg)) => string(last(arg)) end end;

then do

$DataFrames.rename!($x, $pairs_to_str_pairs($(t...)...))

This means you don't have to do MacroTools.postwalk in the parsing stage. See the function make_source_concrete for inspiration. It's used when we have more than one input column, i.e. @rtransform df :x = :a + :b

Edge case: What happens when we want to rename the first column, i.e. @rename df :a = $1. I want this to work, particularly because rename(df, 1 => :a) works. But I don't want to mess around with names(df, 1) in this stage. So its a tough balance. Fortunately we get to point fingers at DataFrames.jl for any limitations.

I've resolved your comments aside from this edge case. Could we handle this case with a modified version
of pairs_to_str_pairs?

function pairs_to_str_pairs(args...) map(args) do arg if !(arg isa Pair) throw(ArgumentError("Non-pair created in @rename")) end if first(arg) isa Int return first(arg) => string(last(arg)) end string(first(arg)) => string(last(arg)) end end

Yes this seems good!

Remember, this is done at call-time, not at the parsing stage.

pdeffebach · 2022-12-08T16:33:21Z

Thanks!

Sorry, I thought you had pushed your changes. Please push them and I will take a look.

pdeffebach · 2022-12-08T22:11:53Z

test/rename.jl

+    df = DataFrame(old_col1 = rand(10), old_col2 = rand([missing;2:10],10),old_col3 = rand(10));
+
+    # rename test set
+    @test @rename(df, :new1 = :old_col1) ≅ rename(df, :old_col1 => :new1)


Can you add some tests with $"a string" = ... i.e. $ on the LHS?

pdeffebach · 2022-12-14T14:41:20Z

@MatthewRGonzalez Is this ready for a review?

MatthewRGonzalez · 2022-12-14T14:47:24Z

@pdeffebach yes, thanks!

pdeffebach · 2023-02-05T23:06:13Z

src/parsing.jl

+        end         
+
+        if first(arg) isa Int            
+            return first(arg) => string(last(arg))


@MatthewRGonzalez

Thank you for your work. One small thing. I think string is too strong here. Here's the issue. :(AsTable(:x)) passes get_column_expr because we allow AsTable to be used as a column identifier identifier. inside transformations. So you end up calling string(:old_col) => string(AsTable(:x))

The problem is that string(AsTable(:x)) gets turned into "AsTable(x)".

The solution is two-fold

Make a new function called get_column_expr_rename which only looks for :x and $(...), no AsTable

Add more error handling in the pairs_to_str_pairs function.

I've done both and added more tests. I've also re-written the docstring.

So I think this is good to merge!

Good catch, @pdeffebach. Thanks for adding the style fixes, error handling, and adding get_column_expr_rename as well!

Matthew Gonzalez added 2 commits November 3, 2022 18:56

add rename macro, docs, and tests

9f9e69f

fix docs

d6f8dcd

MatthewRGonzalez force-pushed the RenameFeature branch from 690c49d to d6f8dcd Compare November 3, 2022 23:39

bkamins reviewed Nov 4, 2022

View reviewed changes

update docs

1c2d233

add helper function in parsing, update macros and tests

3663d39

pdeffebach reviewed Nov 15, 2022

View reviewed changes

revised code and added tests

445be76

pdeffebach reviewed Dec 8, 2022

View reviewed changes

add tests and remove unblock

15b442e

pdeffebach added 5 commits February 5, 2023 17:14

a few styles fixes

aafc837

add error checking

0955d59

small fixes

756fab4

add methods

c53b382

change docstring

9758250

pdeffebach reviewed Feb 5, 2023

View reviewed changes

pdeffebach merged commit 3c5ba5f into JuliaData:master Feb 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add rename macro, docs, and tests #343

add rename macro, docs, and tests #343

MatthewRGonzalez commented Nov 3, 2022 •

edited

Loading

bkamins Nov 4, 2022

bkamins Nov 4, 2022

MatthewRGonzalez Nov 7, 2022

bkamins Nov 7, 2022

pdeffebach commented Nov 8, 2022

MatthewRGonzalez commented Nov 9, 2022 •

edited

Loading

pdeffebach left a comment

pdeffebach Nov 15, 2022

pdeffebach Nov 15, 2022

pdeffebach Nov 15, 2022

pdeffebach Nov 15, 2022

pdeffebach Nov 15, 2022

MatthewRGonzalez Dec 6, 2022 •

edited

Loading

pdeffebach Dec 8, 2022

MatthewRGonzalez Dec 8, 2022

pdeffebach Dec 8, 2022 •

edited

Loading

pdeffebach Nov 15, 2022

MatthewRGonzalez Dec 6, 2022

pdeffebach Dec 8, 2022

pdeffebach commented Dec 8, 2022

pdeffebach Dec 8, 2022

pdeffebach commented Dec 14, 2022

MatthewRGonzalez commented Dec 14, 2022

pdeffebach Feb 5, 2023

MatthewRGonzalez Feb 5, 2023

add rename macro, docs, and tests #343

add rename macro, docs, and tests #343

Conversation

MatthewRGonzalez commented Nov 3, 2022 • edited Loading

New Macros

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdeffebach commented Nov 8, 2022

MatthewRGonzalez commented Nov 9, 2022 • edited Loading

pdeffebach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewRGonzalez Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdeffebach Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdeffebach commented Dec 8, 2022

Choose a reason for hiding this comment

pdeffebach commented Dec 14, 2022

MatthewRGonzalez commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewRGonzalez commented Nov 3, 2022 •

edited

Loading

MatthewRGonzalez commented Nov 9, 2022 •

edited

Loading

MatthewRGonzalez Dec 6, 2022 •

edited

Loading

pdeffebach Dec 8, 2022 •

edited

Loading