Add more splatting and appending of arguments in by #1620

pdeffebach · 2018-12-07T23:11:42Z

This is a first attempt at resolving #1615

It just gets around this by assuming that if you have many different arguments we can vcat them together and pass it off to combine. combine has a pretty good infrastructure for sorting out arguments so I don't think this needs to happen at by.

pdeffebach · 2018-12-07T23:38:10Z

I added the method for f::Any... rather than write a new method because f::Union{Pair, Vector{Pair}}... choose Pair OR Vector{Pair} but does not allow a collection of both.

Unless my interpretation is mistaken, which is likely is.

bkamins · 2018-12-08T21:29:55Z

A small comment (I have not analyzed the code yet): we want combine and by to have the same API so probably combine should get an additional method also and then by implementation would change.

nalimilan · 2018-12-09T14:22:56Z

I think this should be done by combine, as by just passes f to it.

nalimilan · 2018-12-09T14:25:40Z

I added the method for f::Any... rather than write a new method because f::Union{Pair, Vector{Pair}}... choose Pair OR Vector{Pair} but does not allow a collection of both.

You need AbstractVector{<:Pair} (classic issue with Pair).

bkamins · 2018-12-09T15:06:16Z

I think this should be done by combine

Just to make sure I understand you correctly. Do you mean what I have recommended - to have combine handle all required cases consistently with by? And then by would just pass what it gets to combine and leave all the work for combine?

pdeffebach · 2018-12-10T03:59:47Z

Okay I added a method for combine to make this work.

I changed the by method to have a splatted ::Union{Pair, AbstractVector{<:Pair}} but I could presume Milan you want a splatted ::Any... in by?

pdeffebach · 2018-12-10T16:06:11Z

This needs more work. I'm going to see if I can get this to work while also allowing (non-vector) keyword arguments.

nalimilan · 2018-12-10T20:21:57Z

src/groupeddataframe/grouping.jl

@@ -338,7 +338,8 @@ function combine(f::Any, gd::GroupedDataFrame)
    end
 end
 combine(gd::GroupedDataFrame, f::Any) = combine(f, gd)
-combine(gd::GroupedDataFrame, f::Pair...) = combine(f, gd)


If you remove this method, I guess you can also drop Tuple{Vararg{Pair}} from the signature of _combine?

That comment still applies.

yes. It still works without this. It's deleted

It turns out it is needed. combine(f, (:c => sum, :d => mean)) breaks without it.

I thought

combine(gd::GroupedDataFrame, f::Union{Pair, AbstractVector{<:Pair}}...)

would capture it, but it does not. If we wanted to avoid the dispatch we could add a method for combine which collects a tuple of pairs into a Vector.

src/groupeddataframe/grouping.jl

nalimilan · 2018-12-10T20:25:53Z

I changed the by method to have a splatted ::Union{Pair, AbstractVector{<:Pair}} but I could presume Milan you want a splatted ::Any... in by?

I don't think so. Why would it be needed?

pdeffebach · 2018-12-12T20:02:08Z

Okay my strategy for this will be 4 methods for each combine and by.

combine(f::Any, g) # for do syntax with old version
combine(g, f::Any) # for non-do syntax with old version

combine(g, f::{Union{Pair, AbstractVector{<:Pair}, Tuple{<:Pair}}...; kwargs...)  

combine(g, f::NamedTuple) # the one that actually goes to _combine

Here I take the non-keyword arguments and collect them into a named tuple somehow, meaning I move the the autogeneration of variable names to this stage, also making sure I don't overwrite names from the kwargs named tuple.

Does that make sense?

pdeffebach · 2019-01-02T20:42:21Z

I have implemented the above strategy, but only for combine, since we will have to repeat the same logic with by and its easier to focus on just combine while we nail down the strategy.

Rather than splatting a vector of Pair inputs, it uses a reduce(vcat, args) to get a nice vector of Pair arguments regardless of wither you have just a Pair, or a Vector of Pairs or a mixture of the two.

I have a big commented out block that will handle the re-naming in the case of duplicate names, but I need to put more work into understanding the logic of it and what I need to change. The code originally came from the makeunique = true option somewhere.

Any feedback is welcome.

nalimilan · 2019-01-05T16:19:26Z

Makes sense, but note you'll have to handle the case where a single Pair is passed separately, since we support returning a NamedTuple with several fields (and then the names can only be known from firstres). Maybe keep special methods for when you already have an homogeneous named tuple, to avoid doing unnecessary work? Also, better base your work on #1651 as it changes the logic a bit.

pdeffebach · 2019-02-06T23:20:50Z

Rebased and revived this PR. However in the process the _combine step doesn't work. Thankfully after this we can delete all the code for _combine that works with anything other than a named tuple. Not sure how to fix this error and how to figure out which _combine code should be deleted.

nalimilan · 2019-02-07T17:03:46Z

src/groupeddataframe/grouping.jl

-        combine(identity, gd)
-    else
-        combine(values(f), gd)
+    # handle pairs on their own so we can iterate 


That's really too much code for a small feature IMHO. Is there any way to make this simpler? We don't have to allow mixing keyword and positional arguments.

pdeffebach · 2019-02-07T18:02:43Z

That's really too much code for a small feature IMHO. Is there any way to make this simpler? We don't have to allow mixing keyword and positional arguments.

If we don't mix positional and keyword arguments than things become simple again, and I doubt people will miss them. I would prefer to either close this or do a force-push of master to start anew.

It would be nice to allow splatting for multiple arrays in a function call. Then we wouldn't have to worry about supporting vector arguments at all. However this isn't allowed, right?

x = [1, 2, 3]; y = [4, 5, 6]
foo(x..., y...) # foo(1, 2, 3, 4, 5, 6)

pdeffebach · 2019-02-12T22:46:31Z

I have reset this --hard and rebased. Will get a much more minimal implementation together tomorrow. I will

a) make a function with Union{Pair, AbstractVector{<:Pair}... and then call reduce(vcat) on it.
b) delete the method for _combine that this change would make dead code.

pdeffebach · 2019-02-13T20:21:57Z

Okay this PR should be ready for a review! I promise its a lot less to review than last time.

I want to deprecate one method, and thats combine(df, :a, (:b => mean,)). It's only allowed because _combine allows for NTuple{Pair}. But now that using many vectors is flexible, supporting a Tuple argument seems unnecessary.

bkamins · 2019-02-13T22:01:06Z

But now that using many vectors is flexible, supporting a Tuple argument seems unnecessary.

But tuple is lighter than vector so why would we want to prefer vectors over tuples? (but this is a corner case anyway - probably @nalimilan has a stronger opinion here what is right, because he spent many more hours than me on this).

I want to deprecate one method

I could not find the code with the deprecation (but you probably can add it later - first we should decide if we want to drop support for it).

combine(df, :a, (:b => mean,))

Did you rather mean by(df, :a, (:b => mean,))?

Finally - maybe you can add to tests the original reason for this PR, i.e. something like by(df, :x, [:a, :b] .=> mean, :b => std) + as always add negative tests that should fail 😄 (they are really important in long run when we change functionality).

pdeffebach · 2019-02-13T22:05:18Z

thanks, I will add those tests, and try to think of a good failing test.

by and combine should always have the same syntax, I think.

I have not deprecated it and it works now, but is only type-specified at the _combine level, rather than the combine() level. i.e. it fits in as part of combin(gd::AbstractDataFrame, f::Any) method.

I want to deprecate it because if we allow tuple, then we might as well have multiple tuples, along with tuple and scalar...

bkamins · 2019-02-13T22:12:39Z

by and combine should always have the same syntax, I think.

AFAIK by takes AbstractDataFrame and grouping columns to group by and combine takes GroupedDataFrame (and it seems in what you have written you pass a data frame and grouping column).

pdeffebach · 2019-02-13T22:13:32Z

Oh! sorry. yes that was my mistake.

pdeffebach · 2019-06-11T17:25:35Z

I now have the naming working with my (somewhat hacky) solution. This solution seems more elegant than making the type of our Pair vector a small union of a bunch of different things.

I also added some tests that catch what went wrong.

nalimilan · 2019-06-11T19:57:52Z

I think you can just replace p isa Pair{<:Union{Symbol,Integer}} with p isa Pair && first(p) isa Union{Integer,Symbol}.

pdeffebach · 2019-06-11T21:16:11Z

That's a good suggestion, and I have incorporated it.

Things should be good now.

bkamins · 2019-06-11T21:22:08Z

Union{Integer,Symbol} should be ColumnIndex.

pdeffebach · 2019-06-11T21:23:48Z

Thanks for the catch. I've updated it.

pdeffebach · 2019-06-27T18:08:27Z

Can this be merged even though some of the coverage tests didn't pass?

bkamins · 2019-06-27T21:12:02Z

Coverage passes. The problem is that it needs to be merged with master again (sorry for inconvenience).
Also when you merge the CI failures on nightly will get fixed.

…_grouping

pdeffebach · 2019-07-16T18:57:21Z

I think things are correct now?

bkamins · 2019-07-16T19:21:55Z

Thank you for the update.

Let us wait for @nalimilan for a final review, as he is the "groupby" master 😄 (he is off for a few days, but we can go back to this PR very soon).

bkamins · 2019-09-02T22:09:36Z

@nalimilan - how do you see we should proceed with this (given 1.0 release plans?)

nalimilan · 2019-09-03T18:43:13Z

Sorry for the delay. There are still open discussions above. Can you comment on or close them as appropriate? At least the one about the map docstring still applies AFAICT.

nalimilan · 2019-09-03T18:42:31Z

src/groupeddataframe/grouping.jl

            incol = gd.parent[!, first(p)]
            idx = gd.idx[gd.starts]
            outcol = agg(incol, gd)
            return idx, outcol
        else
            fun = do_f(last(p))
-            if p isa Pair{<:ColumnIndex}
+
+             if p isa Pair && first(p) isa ColumnIndex


Suggested change

if p isa Pair && first(p) isa ColumnIndex

if p isa Pair && first(p) isa ColumnIndex

pdeffebach · 2019-10-18T19:17:23Z

Someone posted on discuss here. I need to finish this PR

bkamins · 2019-10-18T20:19:01Z

Would this PR avoid recompilation? (I have posted an answer how the problem on Discourse can be solved using API we have now)

pdeffebach · 2019-10-18T20:38:23Z

No, but it would have avoided that complicated named tuple expression with repeated pairs to the same functions.

bkamins · 2019-12-01T13:33:58Z

This PR might have to be dropped or significantly reviewed if we go for :oldcol => f => :newcol pattern as discussed with @nalimilan.

pdeffebach · 2019-12-01T14:02:46Z

yeah i dropped the ball on this PR, and I apologize for that.

This PR would fall into place really easily if we implemented #2016 and some infrastructure to handle this kind of thing. For example ideally

by(df, :a, Between(:d, :m) => mean)
by(df, :a, [Between(:d, :m), :q] .=> mean)

Should both work via some generic code that is not unique to by.

One benefit of Milan's suggestion to avoid named tuples is that it reduces the gymnastics with arguments, support for which were dropped from this PR because it would have added a lot of boiler-plate code for getting all the different argument formats to play nice.

nalimilan · 2019-12-01T14:47:21Z

Note that I didn't really suggest dropping keyword arguments, rather adding another possible syntax.

bkamins · 2019-12-01T20:45:25Z

Note that I didn't really suggest dropping keyword arguments, rather adding another possible syntax.

This is how I understood this. My point is that it has to be rethought how broadcasting would come into play in :oldcol => f => :newcol syntax as quickly thinking about it @. `oldcols => f => newcols should be used (but I have not analyzed it in detail).

bkamins · 2020-04-04T07:10:35Z

I am closing it as it is now implemented (except for broadcasting of Between etc. which is a separate issue)

bkamins · 2020-04-04T07:11:03Z

@pdeffebach - thank you for raising this functionality. I think it helped a lot to improve the package.

nalimilan reviewed Dec 10, 2018

View reviewed changes

bkamins mentioned this pull request Jan 15, 2019

DataFrames.jl roadmap #1678

Closed

31 tasks

pdeffebach force-pushed the vector_grouping branch from a43f53c to 28a4fa2 Compare February 6, 2019 23:17

nalimilan reviewed Feb 7, 2019

View reviewed changes

pdeffebach closed this Feb 12, 2019

pdeffebach force-pushed the vector_grouping branch from d5301c3 to 359bf31 Compare February 12, 2019 22:27

pdeffebach added 2 commits February 13, 2019 08:43

First restart

7649508

Get tests to pass

9d2aa65

pdeffebach reopened this Feb 13, 2019

add tests for multiple vector inputs

05959f6

Add Tuple argument for explicitness

858ad17

pdeffebach added 2 commits June 11, 2019 16:11

Milan's suggestion of first(p)

dd537b8

Fix short circuit

0b9dba5

add ColumnIndex

3c6633a

pdeffebach mentioned this pull request Jun 11, 2019

Two regex-related items on a wish list #1849

Closed

pdeffebach and others added 2 commits July 16, 2019 11:17

Merge branch 'master' into vector_grouping

efb8ff7

Merge remote-tracking branch 'pdeffebach/vector_grouping' into vector…

4991204

…_grouping

nalimilan reviewed Sep 3, 2019

View reviewed changes

pdeffebach mentioned this pull request Nov 15, 2019

Standardizing working with multiple columns #2016

Closed

bkamins added the non-breaking The proposed change is not breaking label Feb 12, 2020

bkamins closed this Apr 4, 2020

	if p isa Pair && first(p) isa ColumnIndex
	if p isa Pair && first(p) isa ColumnIndex

Add more splatting and appending of arguments in by #1620

Add more splatting and appending of arguments in by #1620

Conversation

pdeffebach commented Dec 7, 2018

pdeffebach commented Dec 7, 2018

bkamins commented Dec 8, 2018 • edited by nalimilan Loading

nalimilan commented Dec 9, 2018

nalimilan commented Dec 9, 2018

bkamins commented Dec 9, 2018

pdeffebach commented Dec 10, 2018

pdeffebach commented Dec 10, 2018

nalimilan Dec 10, 2018

Choose a reason for hiding this comment

nalimilan May 9, 2019

Choose a reason for hiding this comment

pdeffebach May 20, 2019

Choose a reason for hiding this comment

pdeffebach May 21, 2019

Choose a reason for hiding this comment

nalimilan commented Dec 10, 2018

pdeffebach commented Dec 12, 2018

pdeffebach commented Jan 2, 2019

nalimilan commented Jan 5, 2019

pdeffebach commented Feb 6, 2019

nalimilan Feb 7, 2019

Choose a reason for hiding this comment

pdeffebach commented Feb 7, 2019

pdeffebach commented Feb 12, 2019

pdeffebach commented Feb 13, 2019

bkamins commented Feb 13, 2019

pdeffebach commented Feb 13, 2019

bkamins commented Feb 13, 2019

pdeffebach commented Feb 13, 2019

pdeffebach commented Jun 11, 2019

nalimilan commented Jun 11, 2019

pdeffebach commented Jun 11, 2019

bkamins commented Jun 11, 2019

pdeffebach commented Jun 11, 2019

pdeffebach commented Jun 27, 2019

bkamins commented Jun 27, 2019

pdeffebach commented Jul 16, 2019

bkamins commented Jul 16, 2019

bkamins commented Sep 2, 2019

nalimilan commented Sep 3, 2019

nalimilan Sep 3, 2019

Choose a reason for hiding this comment

pdeffebach commented Oct 18, 2019

bkamins commented Oct 18, 2019

pdeffebach commented Oct 18, 2019

bkamins commented Dec 1, 2019

pdeffebach commented Dec 1, 2019

nalimilan commented Dec 1, 2019

bkamins commented Dec 1, 2019

bkamins commented Apr 4, 2020

bkamins commented Apr 4, 2020

bkamins commented Dec 8, 2018 •

edited by nalimilan

Loading