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

[BREAKING] improve naming of ComposedFunction in aggregation #2274

Merged
merged 10 commits into from
Jun 8, 2020

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented May 31, 2020

Fixes change introduced by JuliaLang/julia#35980.

I assume we prefer function rather than ComposedFunction to be used in auto-generated column names. Also this is non-breaking this way.

@nalimilan - I have checked and I think that JuliaLang/julia#35980 does not break aggregation code for composed functions (we still should use a fast branch then), but maybe you will want to take a second look.

@bkamins bkamins added non-breaking The proposed change is not breaking backport labels May 31, 2020
@bkamins bkamins added this to the 1.0 milestone May 31, 2020
@nalimilan
Copy link
Member

Thanks. Actually this is an opportunity to improve the printing of composed functions:

julia> repr(sum  skipmissing)
"sum ∘ skipmissing"

Do you agree we'd better take advantage of this by adding a special method? One issue is that repr uses the full name, which isn't what we do know nor probably what we want (for conciseness):

julia> repr(Base.Libc.realloc  sum)
"Base.Libc.realloc ∘ sum"

So I guess we have to reconstruct the composed name by hand.

@bkamins
Copy link
Member Author

bkamins commented Jun 1, 2020

I was considering it but the problem is that things like: "sum ∘ skipmissing" are long, contain spaces, and many novices will not know how to key-in . That is why I opted for printing what we currently do (i.e. :function). However, if we can find some concise way to print it I would be OK with it. A first idea would be to print e.g. sum_skipmissing - i.e. use _ and do not use at all. What do you think?

@nalimilan
Copy link
Member

Ah, right, maybe using _ is more practical! We could also keep only the first function, and indicate that the other(s) is abbreviated in some way. The most common use case is f ∘ skipmissing and there we don't really care about skipmissing.

@bkamins
Copy link
Member Author

bkamins commented Jun 1, 2020

so you would generate eg :sum_ for sum∘skipmissing?

@nalimilan
Copy link
Member

I don't really know TBH. It's true that the trailing _ isn't very evocative. Maybe sum_skipmissing isn't so bad.

@bkamins
Copy link
Member Author

bkamins commented Jun 1, 2020

OK - I will change it this way. Note that this will also affect multiply nested compositions (but this is rare, so it is probably not a problem).

@bkamins bkamins added breaking The proposed change is breaking. and removed non-breaking The proposed change is not breaking labels Jun 1, 2020
src/other/utils.jl Outdated Show resolved Hide resolved
@bkamins bkamins changed the title Keep naming ComposedFunction :function [BREAKING] Keep naming ComposedFunction :function Jun 1, 2020
src/other/utils.jl Outdated Show resolved Hide resolved
@tkf
Copy link
Contributor

tkf commented Jun 1, 2020

I wonder if it's useful here if julia uses AnonymousFunction <: Function for anonymous functions?

@bkamins
Copy link
Member Author

bkamins commented Jun 1, 2020

I think it would be useful so that we would not need to use the hack with #.

bkamins and others added 4 commits June 1, 2020 23:19
Co-authored-by: Takafumi Arakaki <takafumi.a@gmail.com>
@bkamins
Copy link
Member Author

bkamins commented Jun 4, 2020

OK to merge?
Also we should make a decision if we backport this (the change is breaking, but on the other hand without it DataFrames.jl behaves differently on different versions of Julia).

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.

I think we'd better not backport this, as it could break some workflows. The inconsistency isn't ideal, but at least it will only hit people who upgrade to a newer Julia version.

src/other/utils.jl Outdated Show resolved Hide resolved
test/grouping.jl Outdated Show resolved Hide resolved
bkamins and others added 2 commits June 8, 2020 09:40
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@bkamins bkamins removed the backport label Jun 8, 2020
@bkamins
Copy link
Member Author

bkamins commented Jun 8, 2020

OK - I have removed backport. But this means that maybe we should consider making a release 0.22 when Julia 1.5 is out.

@bkamins bkamins merged commit c5f1bb8 into JuliaData:master Jun 8, 2020
@bkamins bkamins deleted the fix_composedfunction branch June 8, 2020 22:20
@bkamins
Copy link
Member Author

bkamins commented Jun 8, 2020

Thank you!

@bkamins bkamins changed the title [BREAKING] Keep naming ComposedFunction :function [BREAKING] improve naming of ComposedFunction in aggregation Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The proposed change is breaking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants