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 row-wise macros #267

Merged
merged 16 commits into from
Jul 25, 2021
Merged

Add row-wise macros #267

merged 16 commits into from
Jul 25, 2021

Conversation

pdeffebach
Copy link
Collaborator

No description provided.

@pdeffebach
Copy link
Collaborator Author

Just finishing adding the macros, will add tests tomorrow (should be easy, just the same as the @byrow tests).

src/DataFramesMeta.jl Outdated Show resolved Hide resolved
src/DataFramesMeta.jl Outdated Show resolved Hide resolved
@pdeffebach
Copy link
Collaborator Author

Okay I've updated tests and docstrings and a little bit of index.md.

One question for @bkamins and @nalimilan: Should we deprecate

@transform df @byrow 
    ...
end

Given that this can more easily be written as @rtransform? If so, I will add a deprecation message and update the docs, after that this will be ready for merging.

@bkamins
Copy link
Member

bkamins commented Jul 10, 2021

Should we deprecate

@transform df @byrow 
    ...
end

The question is if it hurts to have it. I understand that we keep @byrow for cases when you call @transform with some operations that are intended to be done row-wise and some that are intended to be done columnwise. Is this a correct understanding? (as if we keep @byrow anyway then I think we can leave it)

@pdeffebach
Copy link
Collaborator Author

Sorry, I should have been clearer. I mean

@transform df @byrow begin 
    :y = f(:x)
    :z = g(:x)
end

That is, when @byrow is an "outer" flag, all operations are row-wise. This is exactly equivalent to @rtransform begin ... end

@bkamins
Copy link
Member

bkamins commented Jul 10, 2021

Yes - I have understood it. And I meant that as long as we accept @byrow also as an inner flag (per transformation) I am OK to keep it. So - as you prefer. I would make a decision based on the fact if it would simplify the code if you removed it.

docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
docs/src/index.md Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
@@ -595,6 +621,21 @@ function subset!_helper(x, args...)
end
end

function rsubset!_helper(x, args...)
Copy link
Member

Choose a reason for hiding this comment

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

Would there be a way to avoid duplicating these helper functions, for example by passing subset or subset! to a more generic one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that't what DataFrameMacros does. I will do this in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

Why not do it now? :-p

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I've simplified the @byrow check but it's hard to actually make everything in a coherent function

  1. The ugly t = (fun_to_vec(ex; no_dest=true, outer_flags=outer_flags) for ex in exprs) can't be gotten rid of yet because we still need to support some deprecated functionality in @combine.
  2. There are lots of exceptions, for example @subset requires a keyword argument, @orderby doesn't even call a DataFrames function. Writing a function for these cases would be just as easy as writing out the expression.

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 I think this is also ready for merging!

@nalimilan @bkamins as Milan noted there is some technical debt here but it can be re-worked in a future PR.

@@ -595,6 +621,21 @@ function subset!_helper(x, args...)
end
end

function rsubset!_helper(x, args...)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that't what DataFrameMacros does. I will do this in a future PR.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2021

As commented in #270 maybe we need @filter as @rsubset is not going to allow for filtering of groups?

@pdeffebach
Copy link
Collaborator Author

Maybe I should restrict the row-wise macros to only work on AbstractDataFrames in order to avoid confusion.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2021

I think it is ok for row-wise macros to work also on GroupedDataFrame. However, given the questions today during the workshop - it is crucial to explain the row-wise vs whole-column behavior. It seems it is a super confusing distinction for newcomers.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2021

If I do not do a final review of this PR in 2 days from now please ping me (I cannot do it now).

@pdeffebach
Copy link
Collaborator Author

Yes I'm sorry I wasn't there to help field questions. I should have offered.

@bkamins
Copy link
Member

bkamins commented Jul 20, 2021

Thank you for posting this. Actually I have not expected so many questions (but probably it is a good sign that there were so many questions). I was swamped during the first hour, but then I learned how to sort them.

@pdeffebach
Copy link
Collaborator Author

Thank you for the review!

I have responded to comments.

@pdeffebach pdeffebach closed this Jul 23, 2021
@pdeffebach pdeffebach reopened this Jul 23, 2021
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
src/macros.jl Outdated Show resolved Hide resolved
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@pdeffebach
Copy link
Collaborator Author

Sorry about the typos. This is ready to be merged, then.

@pdeffebach pdeffebach mentioned this pull request Jul 24, 2021
@pdeffebach
Copy link
Collaborator Author

Okay the merge was actually very easy and there were no conflicts.

I will merge this!

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.

None yet

3 participants