Add `unique` macros #340

MatthewRGonzalez · 2022-10-07T19:20:14Z

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

New Macros

The following macros are added

@unique and @runique
@unique! and @runique!

These macros extend the functionality of DataFrames.unique and DataFrames.unique! to the DataFramesMeta.jl ecosystem. Both general and row-wise versions are included for consistency with similar macro functions.

Tests

General tests are added in unique.jl
Tested with byrow in byrow.jl

pdeffebach

Thanks!

I think we should change the implementation a bit to focus on select and mirror the internals of DataFrames.jl here.

Also, this means that

@unique(df, [:a, :b])

means something very different from

unique(df, [:a, :b]

that's okay, but this should be documented.

pdeffebach · 2022-10-14T20:34:42Z

src/macros.jl

+unique(df, :x => ByRow((x,y) -> x + y))
+```
+
+a transformation which cannot be conveniently expressed


You can delete this line.

pdeffebach · 2022-10-14T20:37:10Z

src/macros.jl

+"""
+    @unique(x, args)
+
+Select unique rows in `AbstractDataFrame`s.


Can you also communicate that args can be transformations?

What happens if multiple arguments are passed? Currently it errors

julia> @unique(df, :x, :b) ERROR: MethodError: no method matching unique(::DataFrame, ::Symbol, ::Symbol)

Maybe just allow one argument for consistency? Or we need to do some other implementation which uses select under the hood and allows multiple arguments.

I think that's the best way forward.

pdeffebach · 2022-10-14T20:45:22Z

src/macros.jl

+    exprs, outer_flags = create_args_vector(args...; wrap_byrow=true)
+    t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs)
+    quote
+        $DataFrames.unique($x, $(t...))


@bkamins Looking at the docs for unique, the fact that transformations can be passed to unique is not explicitly documented.
Instead of using unique, do

df2 = select(df, t...; copycols = false) rowidxs = (!).(nonunique(df2)) df[rowidxs, :]

cols in unique is documented as:

in selected columns or their transformations

so it explicitly mentions that transformations are allowed. But maybe a better wording could be used (this was written a long time ago, when we have a much less precise vocabulary in DataFrames.jl 😄). If you have a proposal out of hand then please comment.

pdeffebach · 2022-10-14T20:46:32Z

test/unique.jl

+
+    @test @unique(df, $:A .+ :B) ≅ unique(df, [:A, :B] => (x, y) -> x .+ y)
+    @test @unique(df, $:A.+$:B) ≅ unique(df, [:A, :B] => (x, y) -> x .+ y)
+    @test @unique(df, :A.+$:B) ≅ unique(df, [:A, :B] => (x, y) -> x .+ y)


Can you test with string column names?

pdeffebach · 2022-10-14T20:47:07Z

test/unique.jl

+    df = DataFrame(A=[1, 1, 3, missing], B=[1, 1, 2, 1])
+
+    d = @unique df begin
+        :A .+ :B


You didn't test with multiple arguments ;)

julia> @unique df begin :x :b end ERROR: MethodError: no method matching unique(::DataFrame, ::Symbol, ::Symbol)

MatthewRGonzalez · 2022-10-16T16:29:14Z

Hi, @pdeffebach. Thank you for your thoughtful feedback!

I've addressed these requested changes in 88a779b. In this commit, I implemented the select method you described above and also added a condition to handle cases where t... is not present. This additional condition allows @unique(df) to operate in the same manner as unique(df)

The documentation is updated to describe the difference between @unique and unique, and also describes that arguments can be transformations. Tests are also updated to include multiple arguments and string column names.

Please note that I used gensym in the helper functions in an attempt to keep the macros hygienic. If there's a more appropriate or concise method that you'd prefer, please let me know!

pdeffebach · 2022-10-19T13:12:35Z

src/macros.jl

+    t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs)
+    tmp = gensym("tmp")
+    quote            
+        $(tmp) = DataFrames.select($x, $(t...); copycols = false)


Can you put this all into a separate function? That way we can dispatch on ::AbstractDataFrame and avoid thinking about GroupedDataFrames

as in

function DataFramesMeta.make_unique(df, args...) ... end function unique_helper(x, args...) ... quote $make_unique($x, $t...) end end

pdeffebach · 2022-10-19T13:13:40Z

src/macros.jl

+to given combinations of values in selected columns or their transformation. 
+`args` can be any column selector or transformation accepted by `select`.
+
+The implementation of `@unique` differs from the `unique` function in DataFrames.jl. 


This paragraph is too technical.

Please create a ### Details section beneath ### Returns to discuss the implementation. The main thing the user needs to know is that

unique(df, [:a, :b])

is different from @unique(df, [:a, :b])

No problem. Also, I want to note that in the current version, @unique(df, [:a,:b]) will error, while @unique(df, $[:a,:b]) works. Would you like me to adjust the function such that @unique(df, [:a,:b]) works in the same manner as @unique(df, $[:a,:b]). Or, should I alter the function to throw an error similar to that which is observed via @select(df, [:a,:b]):

# example df = DataFrame(x = ones(10), z = rand(0.0:1.0,10), d = [1,1,1,1,1,2,2,2,2,2]) unique(df, [:x, :z, :d]) @unique(df, $[:x, :z, :d]) @unique(df, [:x, :z, :d]) ERROR: ArgumentError: length 3 of vector returned from function #5 is different from number of rows 10 of the source data frame. select(df, [:x,:z]) @select(df, [:x,:z]) ERROR: LoadError: ArgumentError: Malformed expression in DataFramesMeta.jl macro```

Okay I have the solution.

Follow the implementation of @orderby and use gensym_names = true. That will fix the errors.

I'm happy to be wrong, but I find that@orderby(df, [:x, :z, :d]) results in the same error as above on v0.12.0, and that following the implementation of @orderby doesn't seem to resolve these errors.

That's fine. If the user understands that it uses select machinery under the hood, and we communicate that unique(df, [:x, :y, :z]) is fundamentally different than @unique(df, [:x, :y, :z]) then we are fine.

The problem is that

@unique(df, [:x, :y, :z])

creates a three element array and tries to make that a column. If the data frame is a different length, it will error.

The major problem here is that @unique(df, [:x, :y, :z]) will work if the DataFrames has 3 rows...

I think this is okay though. As long as users know what it's doing.

I wonder if this deviates enough from DataFrames.unique to merit a new name? Similar to sort and @orderby. Maybe we should call this @distinct? I hope we don't get sued by rstudio for that.

cc @bkamins @nalimilan

So the deviation is that you want to allow @distinct(df, :a, :b, :c)? While @distinct(df, $[:a, :b, :c]) is required as @distinct(df, [:a, :b, :c]) errors? Then I think it would be better to have a new name.

Yes.

I agree, a new name is probably better here. @MatthewRGonzalez can you change to @distinct?

Sure. I'll commit with some updated documentation later on.

pdeffebach · 2022-10-19T13:13:57Z

src/macros.jl

+and 
+
+```
+@unique df :x + :y


Suggested change

@unique df :x + :y

@unique(df, :x + :y)

pdeffebach · 2022-10-19T13:16:52Z

src/macros.jl

+     │ Int64  Int64
+─────┼──────────────
+   1 │     1     5
+````


Suggested change

````

pdeffebach · 2022-10-19T13:18:23Z

src/macros.jl

+    t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs)
+    tmp = gensym("tmp")
+    quote                    
+        if isempty($(args))


Having a separate function where we re-implement DataFrames.unique also helps reduce code-duplication.

pdeffebach

Looks good!

Docs could be re-written a little but we can do that later.

Happy to merge after you make the following small changes.

pdeffebach · 2022-10-27T00:28:12Z

src/macros.jl

+of `df` are thus determined by this intermediate data frame. This focus on `select` allows 
+for multiple arguments to be passed conveniently in the form of column names or transformations.
+
+Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $[:x,:y])`


Suggested change

Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $[:x,:y])`

Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $DOLLAR[:x,:y])`

pdeffebach · 2022-10-27T00:28:30Z

src/macros.jl

+of `df` are thus determined by this intermediate data frame. This focus on `select` allows 
+for multiple arguments to be conveniently passed in the form of column names or transformations.
+
+Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $[:x,:y])`


Suggested change

Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $[:x,:y])`

Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $DOLLAR[:x,:y])`

pdeffebach · 2022-10-27T00:28:43Z

src/macros.jl

+end
+
+"""
+    rdistinct(d, args...)


Suggested change

rdistinct(d, args...)

rdistinct!(d, args...)

pdeffebach · 2022-10-27T00:28:53Z

src/macros.jl

+     │ Int64  Int64
+─────┼──────────────
+   1 │     1     5
+````


Suggested change

````

pdeffebach · 2022-10-27T00:38:03Z

JK I figured out how to push to this branch so I will merge when tests pass.

Thanks for your hard work on this!

MatthewRGonzalez · 2022-10-27T01:20:14Z

@pdeffebach It looks like the checks failed due to issues with create_args_vector(). I'm thinking this is due to my use of create_args_vector() instead of get_df_args_kwargs() introduced in #323. I'll resolve this soon. Sorry for not catching this sooner!

pdeffebach · 2022-10-27T13:42:22Z

Tests pass locally... which is confusing.

If you feel comfortable rebasing and changing the logic of the macro to mirror the new implementations of @select etc. that might help.

But rebasing is hard and you've done a lot of work on this so far, so I can take it from here if you need me to.

bkamins · 2022-10-27T14:10:43Z

But rebasing is hard and you've done a lot of work on this so far, so I can take it from here if you need me to.

Rebasing made easy is:

on your local git squash all commits in the PR into a single commit
rebase

The point is that if you do it this way you only need to resolve conflicts once and against the final version of the code. This is much easier than sequentially resolving all commits.

…itional tests Rename to , add helper functions, and update documentation doc fixes small fixes

MatthewRGonzalez · 2022-10-27T17:05:34Z

@pdeffebach I followed the advice above to update this PR. The tests pass locally after rebasing and I've updated the macro logic (e04fb01) to mirror the new implementations of @select etc. I apologize for the additional clutter. Please let me know if this is what you had in mind!

bkamins · 2022-10-27T19:54:02Z

@pdeffebach can check best the details of the implementation, but I would like to thank you for an excellent and big PR (and tests now also pass CI 😄).

pdeffebach · 2022-10-28T14:33:04Z

Thanks!

MatthewRGonzalez mentioned this pull request Oct 7, 2022

Adding preliminary draft of the unique macro (with variations) #339

Closed

2 tasks

pdeffebach requested changes Oct 14, 2022

View reviewed changes

MatthewRGonzalez requested a review from pdeffebach October 16, 2022 16:46

pdeffebach reviewed Oct 19, 2022

View reviewed changes

MatthewRGonzalez requested a review from pdeffebach October 26, 2022 01:46

pdeffebach requested changes Oct 27, 2022

View reviewed changes

MatthewRGonzalez force-pushed the UniqueFeature branch from c46e8ea to 13632ed Compare October 27, 2022 15:54

Matthew Gonzalez added 2 commits October 27, 2022 12:31

Adjust formatting for PR

4a52146

Updated macro to focus on select,updated documentation, and added add…

940f1e4

…itional tests Rename to , add helper functions, and update documentation doc fixes small fixes

MatthewRGonzalez force-pushed the UniqueFeature branch from 13632ed to 940f1e4 Compare October 27, 2022 16:35

update functions after squash and rebase

e04fb01

pdeffebach merged commit 6739447 into JuliaData:master Oct 28, 2022

MatthewRGonzalez deleted the UniqueFeature branch October 28, 2022 16:17

	Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $[:x,:y])`
	Users should be cautious when passing function arguments as vectors. E.g., `@distinct(df, $DOLLAR[:x,:y])`

Add unique macros #340

Add unique macros #340

Conversation

MatthewRGonzalez commented Oct 7, 2022

New Macros

Tests

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

Choose a reason for hiding this comment

MatthewRGonzalez commented Oct 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewRGonzalez Oct 20, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

pdeffebach commented Oct 27, 2022

MatthewRGonzalez commented Oct 27, 2022 • edited Loading

pdeffebach commented Oct 27, 2022

bkamins commented Oct 27, 2022

MatthewRGonzalez commented Oct 27, 2022

bkamins commented Oct 27, 2022

pdeffebach commented Oct 28, 2022

Add `unique` macros #340

Add `unique` macros #340

MatthewRGonzalez Oct 20, 2022 •

edited

Loading

MatthewRGonzalez commented Oct 27, 2022 •

edited

Loading