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 a @passmissing flag, only with @byrow for now. #272

Merged
merged 9 commits into from Jul 31, 2021

Conversation

pdeffebach
Copy link
Collaborator

No description provided.

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
@bkamins
Copy link
Member

bkamins commented Jul 31, 2021

Also index.md needs an update I think (as it currently mentions passmissing). Where is the implementation? I understand you will add it later?

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! @bkamins I just pushed the index.md stuff as well.

So all the moving parts are done and if it looks good it can be merged.

@@ -21,6 +21,7 @@ In addition, DataFramesMeta provides
* `@eachrow` and `@eachrow!` for looping through rows in data frame, again with high performance and
convenient syntax.
* `@byrow` for applying functions to each row of a data frame (only supported inside other macros).
* `@passmissing` for propagating missing values inside DataFramesMeta.jl transformations.
Copy link
Member

Choose a reason for hiding this comment

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

in line 6: DataFramesMeta.jl (I cannot correct it in the editor)

@@ -21,6 +21,7 @@ In addition, DataFramesMeta provides
* `@eachrow` and `@eachrow!` for looping through rows in data frame, again with high performance and
convenient syntax.
* `@byrow` for applying functions to each row of a data frame (only supported inside other macros).
* `@passmissing` for propagating missing values inside DataFramesMeta.jl transformations.
Copy link
Member

Choose a reason for hiding this comment

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

row-wise transformations?

Missings.jl provides the `passmissing` function-wrapper to help get around these
roadblocks: `passmissing(f)(args...)` will return `missing` if any of `args` is
missing. Similarly, DataFramesMeta.jl provides the `@passmissing` function to wrap
the anonymous functions created by DataFramesMeta.jl in `Missings.passmissing`.
Copy link
Member

Choose a reason for hiding this comment

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

row-wise

function check_macro_flags_consistency(exprflags)
if exprflags[PASSMISSING_SYM][]
if !exprflags[BYROW_SYM][]
s = "The `@passmissing` flag is currently only allowed with the `@byrow` flag"
Copy link
Member

Choose a reason for hiding this comment

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

do you test against this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

fun = :(ByRow($fun))
if exprflags[BYROW_SYM][]
if exprflags[PASSMISSING_SYM][]
fun = :(ByRow(Missings.passmissing($fun)))
Copy link
Member

Choose a reason for hiding this comment

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

looks good performance wise :).

Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

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

Looks good. I approve, but after fixing my minor comments you can ping me and I can have a look again.

pdeffebach and others added 2 commits July 31, 2021 12:19
@pdeffebach
Copy link
Collaborator Author

Thanks!

@pdeffebach pdeffebach merged commit a08e731 into JuliaData:master Jul 31, 2021
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

2 participants