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

Allow AsTable on RHS in transformations #307

Merged
merged 15 commits into from
Dec 21, 2021

Conversation

pdeffebach
Copy link
Collaborator

After the recent improvements to AsTable in DataFrames, I guess it's time to make it easier to work with this construct in DataFramesMeta.jl.

No tests added yet.

@bkamins
Copy link
Member

bkamins commented Nov 10, 2021

What would be the test cases for this?

@pdeffebach
Copy link
Collaborator Author

a row-wise sum

cols = [:a, :b, :c, :d]
@rtransform df :y = sum(AsTable(cols))

@bkamins
Copy link
Member

bkamins commented Nov 10, 2021

Nice - probably documentation should be updated (or does it already have it?)

@pdeffebach
Copy link
Collaborator Author

No I haven't done any of that yet. I'll ping you when its ready.

@pdeffebach
Copy link
Collaborator Author

Okay I've added enough stuff so that you can see the API


julia> df = DataFrame(rand(10, 10), :auto);

julia> @rselect df :sum_all = sum(AsTable(names(df)))
10×1 DataFrame
 Row │ sum_all 
     │ Float64 
─────┼─────────
   1 │ 4.19633
   2 │ 4.69652
   3 │ 2.96451
   4 │ 4.32973
   5 │ 4.69788
   6 │ 3.68512
   7 │ 6.47631
   8 │ 5.80387
   9 │ 4.92043
  10 │ 4.83637

@pdeffebach
Copy link
Collaborator Author

Okay ready for a review @bkamins @nalimilan

If only to check if the API makes sense.

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few stylistic comments.

@pdeffebach
Copy link
Collaborator Author

Thanks for the review!

I have addressed all comments. I also got rid of the code example in the table because I couldn't figure out a way to get it to work.

I think this is ready to merge?

@bkamins
Copy link
Member

bkamins commented Dec 11, 2021

Looks good. I made some cosmetic comments.

@@ -525,11 +525,165 @@ To reference columns with more complicated expressions, you must wrap column ref
@transform df :a + $(get_column_name(x))
```

## Constructing multi-column arguments and `src => fun => dest` calls using `$`
## Operations with multiple columns at once using `AsTable` inside operations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why not put this section right after the one about @astable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Re-arranged.

Copy link
Collaborator Author

@pdeffebach pdeffebach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. I addressed all comments.

@@ -525,11 +525,165 @@ To reference columns with more complicated expressions, you must wrap column ref
@transform df :a + $(get_column_name(x))
```

## Constructing multi-column arguments and `src => fun => dest` calls using `$`
## Operations with multiple columns at once using `AsTable` inside operations
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Re-arranged.

pdeffebach and others added 2 commits December 18, 2021 20:20
Co-authored-by: Bogumił Kamiński <bkamins@sgh.waw.pl>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Copy link
Collaborator Author

@pdeffebach pdeffebach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay ready for review / merging.

@bkamins
Copy link
Member

bkamins commented Dec 19, 2021

I went through the comments and resolved all that I thought were addressed. I left one related to indentation (a minor issue) + one regarding the order of presentation of things (so that @nalimilan can check if now we have what he wants).

Thank you for working on thins!

@pdeffebach
Copy link
Collaborator Author

Awesome! Thanks for all the hard work reviewing this.

@pdeffebach pdeffebach merged commit 2aa1b98 into JuliaData:master Dec 21, 2021
@pdeffebach pdeffebach deleted the astable_rhs branch December 21, 2021 16:39
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

Successfully merging this pull request may close these issues.

3 participants