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

improve naming of anonymous functions in aggregate #1576

Merged
merged 2 commits into from Nov 1, 2018

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Oct 27, 2018

October is about to end, so I proposed to solve #1276.

I use standard DataFrame constructor machinery to generate names. The default suffix for lambda function is λ.

Also added makeunique keyword argument as it was missing (as here I guess we should set it to true it by default).

I do not know a better approach to test if a function is anonymous than by checking if first character of its name is # (maybe there is a cleaner solution to this?).

@nalimilan
Copy link
Member

Thanks. I guess detecting anonymous functions that way is fine.

I'm not sure using λ for this is a good idea given that it cannot be typed directly at the keyboard. We always provide ASCII variants when using Unicode identifiers, and here there is no ASCII way to access that column. Why not use f instead (or any other ASCII name)?

@@ -376,11 +376,16 @@ function aggregate(d::AbstractDataFrame,
aggregate(groupby(d, cols, sort=sort), fs)
end

function dfnameof(f)
Copy link
Member

Choose a reason for hiding this comment

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

Why df? Sounds like "data frame".

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the reason because I want to get a name of f for internal DataFrames.jl purposes. Do you have any suggestion what would be a good name.

I also can change it to an anonymous function call in line 385 if this is preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe funname or something like that? Just bikeshedding.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@bkamins
Copy link
Member Author

bkamins commented Oct 29, 2018

I use λ because:

  • this reduces the probability of function name clash;
  • it is short (e.g. when printed);
  • it produces a valid identifier that can be typed in in any Julia editor and Julia REPL.

If we want ASCII, then maybe lambda would be good (I do not like f because of the name clash possibility)?

@nalimilan
Copy link
Member

I'm not a fan of λ/lambda as it's not the most common/explicit term for anonymous functions, especially for people not coming from CS. AFAICT we almost always say "anonymous function" in Julia.

A few ideas: fun, function, agg

@bkamins
Copy link
Member Author

bkamins commented Oct 29, 2018

function is OK, as no function will have this name, fun and agg are pretty common when you check students' homework 😄.

@quinnj
Copy link
Member

quinnj commented Oct 29, 2018

FWIW, I like lambda.

@bkamins
Copy link
Member Author

bkamins commented Nov 1, 2018

I merge function version, as it guarantees not to conflict with an actual function name.

@bkamins bkamins merged commit da48dcb into JuliaData:master Nov 1, 2018
@bkamins bkamins deleted the aggregate_lambda branch November 1, 2018 12:12
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