Add @byrow attempt 2 #250

pdeffebach · 2021-05-28T01:48:35Z

This supercedes #239

Here is a docstring indicating how @byrow works

@transform uses the syntax @byrow to wrap transformations in
the ByRow function wrapper from DataFrames, enabling broadcasting
and more. For example, the call

@transform(df, y = @byrow :x == 1 ? "true" : "false)

becomes

transform(df, :x => ByRow(x -> x == 1 ? "true", "false") => :y)

a transformation which cannot be conveniently expressed
using broadcasting.

To avoid writing @byrow multiple times when performing multiple
transformations by row, @transform allows @byrow at the
beginning of a block of transformations. All transformations
in the block will operate by row.

julia> using DataFramesMeta

julia> df = DataFrame(A = 1:3, B = [2, 1, 2]);

julia> @transform df z = @byrow :A * :B
3×3 DataFrame
 Row │ A      B      z
     │ Int64  Int64  Int64
─────┼─────────────────────
   1 │     1      2      2
   2 │     2      1      2
   3 │     3      2      6

julia> @transform df @byrow begin
           x = :A * :B
           y = :A == 1 ? 100 : 200
       end

3×4 DataFrame
 Row │ A      B      x      y
     │ Int64  Int64  Int64  Int64
─────┼────────────────────────────
   1 │     1      2      2    100
   2 │     2      1      2    200
   3 │     3      2      6    200

The implementation is a bit clunky with create_args_vector returning a tuple of the vector of arguments and whether to wrap in ByRow. This should be less clunky after we clean up the @combine code, which I will do in a later PR. So I accumulate a little bit of technical debt I promise I will pay off in the future.

I have added tests for @transform, but not others, until we find all the edge cases we need. Overall I am pleased with this PR. I think the work in #245 made this really easy. It also preserves the snappy function parsing in #221, though I still have to add those tests.

All things considered, this is ready for a review @nalimilan and @bkamins.

bkamins · 2021-05-28T07:20:23Z

Thank you for working on it. Note that CI fails.

src/macros.jl

bkamins · 2021-05-28T07:29:05Z

test/dataframes.jl

+        y = [:v, :w, :x, :y, :z],
+        c = [:g, :quote, :body, :transform, missing])
+
+    @test @transform(df, n = @byrow :i).n == df.i


Maybe add some tests checking the whole output DataFrame not only its specific columns.
Also do we support GroupedDataFrame here?

I have updated tests to be more reflective of the types of cases we are concerned with.

I will updated the GroupedDataFrame tests once we think these are good. They are in a different file.

pdeffebach · 2021-05-29T18:47:28Z

Okay I've added a very lengthy documentation section in index.md.

Could use an approval of the tests, any comments on the API as discussed in index.md and the @transform docstring. and then I will get to work adding tests everywhere and cleaning up the documentation.

bkamins · 2021-05-29T20:37:49Z

docs/src/index.md

@@ -268,6 +268,178 @@ df2 = @eachrow df begin
 end
 ```

+## Row-wise transformations with `@byrow`
+
+DataFrames provides the function-wrapper `ByRow`. `ByRow(f)(x, y)`


I am not sure what style you follow in DataFramesMeta.jl, but in DataFrames.jl we always add .jl to package names

added .jl but will make a point to do this before 1.0.

docs/src/index.md

bkamins · 2021-05-29T20:39:36Z

docs/src/index.md

+```
+
+!!! note 
+    Unlike `@.`, `@byrow` is not a "real" macro and cannot be used outside of 


the question is what are the parsing rules for @byrow? does it take exactly one expression that follows it?

I included a more detailed discussion in updated docs. Hopefully it's clear.

bkamins · 2021-05-29T20:40:20Z

docs/src/index.md

+### Comparison with `@eachrow`
+
+In previous versions of DataFramesMeta, `@eachrow` was named `@byrow`. 
+This version of `@byrow` is deprecated, but the syntax can be used


"the syntax" - which syntax?

docs/src/index.md

bkamins · 2021-05-29T20:41:41Z

docs/src/index.md

+```
+
+The function `*` is applied by-row. But the result of those operations
+is not stored in a new vector. Additionally, `@eachrow` and `@eachrow!`


where is it stored then? (or not stored unless stored explicitly?)

it's not stored. It's literally just a for-loop. Hopefully the re-write is clear.

docs/src/index.md

bkamins · 2021-05-29T20:45:56Z

docs/src/index.md

+and broadcasts that function. Consequently, it does not broadcast
+objects that are referenced which are not columns. 
+```julia
+@with df @byrow :x + [5, 6]


will this always error? What if column :x would be a vector of two element vectors?

Yes, I have clarified in the example.

bkamins · 2021-05-29T20:46:29Z

docs/src/index.md

+```
+will error. On the other hand
+```julia
+@with df @. :x + [5, 6]


It will only work if :x has 2 elements - right?

yes. I have added more detail.

docs/src/index.md

pdeffebach · 2021-05-29T21:48:29Z

Thanks a ton for reading all this.

I re-wrote a lot of this documentation for clarity based on your comments. Hopefully it's more complete now.

pdeffebach · 2021-05-29T22:03:21Z

It's worth noting that the special behavior related to NamedTuples and AsTable doesn't get referenced here because DataFramesMeta doesn't support AsTable yet.

bkamins · 2021-06-02T07:03:23Z

docs/src/index.md

+used, functions do not take advantage of the grouping, so the 
+behavior of `@transform(df, y = @byrow f(:x))` and 


I add this comment as with @combine the result would be different:

Suggested change

used, functions do not take advantage of the grouping, so the

behavior of `@transform(df, y = @byrow f(:x))` and

used, functions do not take advantage of the grouping, so

for example the result of `@transform(df, y = @byrow f(:x))` and

Ah good catch. Too niche to mention in the docs though.

bkamins · 2021-06-02T07:03:42Z

docs/src/index.md

+
+### Comparison with `@eachrow`
+
+In previous versions of DataFramesMeta, `@eachrow` was named `@byrow`. 


Suggested change

In previous versions of DataFramesMeta, `@eachrow` was named `@byrow`.

In previous versions of DataFramesMeta.jl, `@eachrow` was named `@byrow`.

docs/src/index.md

pdeffebach · 2021-06-03T18:19:29Z

Thanks for the detailed review!

bkamins · 2021-06-03T18:47:28Z

Let us wait for @nalimilan to have a look at it now.

nalimilan

Cool! I've made comments mainly about the docs. Overall I think it would make sense to put less stress on the technical details to target "basic" users.

I also wonder whether @byrow z = :x * :y shouldn't be allowed as it seems a bit simpler not to put @byrow in the middle of the transformation. It's also more consistent with @byrow begin... end. What do you think?

docs/src/index.md

nalimilan · 2021-06-05T16:19:34Z

docs/src/index.md

@@ -268,6 +268,228 @@ df2 = @eachrow df begin
 end
 ```

+## Row-wise transformations with `@byrow`


Mention @byrow at the top of the file?

Rather than starting the section with technical details, it would be more user-friendly to say what @byrow does first, then show examples, and only then mention ByRow and the fact that @byrow isn't a real macro.

I think the second paragraph still applies: it would be nice to start with a sentence or two saying that @byrow allows writing code that is applied to each row instead of having to vectorize it.

docs/src/index.md

src/parsing.jl

test/dataframes.jl

src/macros.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

pdeffebach · 2021-06-05T20:59:09Z

I also wonder whether @byrow z = :x * :y shouldn't be allowed as it seems a bit simpler not to put @byrow in the middle of the transformation. It's also more consistent with @byrow begin... end. What do you think?

No, I don't think we should do this. It breaks the mapping to src => fun => dest logic. The thing on the RHS becomes the fun, which is where the ByRow goes. Plus we have to support @byrow on the RHS because the RHS gets sent to the same processing that stuff in @where goes (without no RHS).

w.r.t. detailed technical docs vs introductory docs, if we were to make a docstring for @byrow, how would that work? We could make @byrow a macro the same as @with df @byrow begin ... end, but that seems a little redundant.

EDIT: Unfortunately @byrow doesn't error on the current release. So we have to have a few months where it errors before we can add @byrow back in as a real macro.

nalimilan · 2021-06-06T20:51:39Z

No, I don't think we should do this. It breaks the mapping to src => fun => dest logic. The thing on the RHS becomes the fun, which is where the ByRow goes. Plus we have to support @byrow on the RHS because the RHS gets sent to the same processing that stuff in @where goes (without no RHS).

Well the mapping isn't 1-to-1 already: while the LHS corresponds to src, the RHS completely mixes fun and dest. I get that in terms of implementation, @byrow z = x + y may be more complex to support, but in terms of API I'm not sure it's really inferior. (That said, it could be added later.)

w.r.t. detailed technical docs vs introductory docs, if we were to make a docstring for @byrow, how would that work? We could make @byrow a macro the same as @with df @byrow begin ... end, but that seems a little redundant.

EDIT: Unfortunately @byrow doesn't error on the current release. So we have to have a few months where it errors before we can add @byrow back in as a real macro.

Can't you define a dummy macro that throws an error?

pdeffebach · 2021-06-06T21:12:57Z

@byrow z = x + y may be more complex to support, but in terms of API I'm not sure it's really inferior. (That said, it could be added later.)

This is a good point. I will think about it. Maybe the implementation is easy.

Can't you define a dummy macro that throws an error?

Yeah that sounds good. I will do that and switch the docstrings around.

pdeffebach · 2021-06-09T14:36:52Z

Okay I'm going to allow

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

because with the @transform df @byrow begin ... end implemenation we allow

@transform df @byrow y = f(:x)

so it would be weird to allow that and not the same for multiple arguments.

docs/Project.toml

pdeffebach · 2021-06-13T22:11:38Z

Okay two substantive changes to the API

@transform df y = @byrow :x now errors, in favor of @transform df @byrow y = :x
Because a lot of this is implemented with recursion, I add a few checks to stop people from doing @transform @byrow @byrow @byrow y = :x

I added docstrings to all the relevant macros (except @combine and @by, since @byrow is useless there.) So in theory this PR is ready to be merged. All the work is done.

Could use a review to go over the docstrings, I guess. But I'm getting anxious to merge this into master.

bkamins

Fantastic work.

src/parsing.jl

test/grouping.jl

src/macros.jl

Co-authored-by: Milan Bouchet-Valat <[email protected]>

pdeffebach

Thanks for the review!

I have incorporated the requested changes.

nalimilan

Cool!

src/macros.jl

pdeffebach added 4 commits May 27, 2021 17:37

add @byrow attempt 2

dc88ed8

docstring in transform

33248f7

ready for review

2112f4c

small fix

d61df1a

bkamins reviewed May 28, 2021

View reviewed changes

src/macros.jl Show resolved Hide resolved

bkamins reviewed May 28, 2021

View reviewed changes

src/macros.jl Outdated Show resolved Hide resolved

bkamins reviewed May 28, 2021

View reviewed changes

src/macros.jl Outdated Show resolved Hide resolved

bkamins reviewed May 28, 2021

View reviewed changes

pdeffebach mentioned this pull request May 28, 2021

WIP Add @byrow #239

Closed

pdeffebach added 4 commits May 28, 2021 18:33

remove deprecated tests

d3e401b

better tests

eab9743

long docs

f4be9f0

more docs

61d96f4