Make order of arguments for groupby consistent with other iterators.

[lendle]

This switches the order of the function and collection arguments in groupby so that it is consistent with the imap and iterate iterators and higher order functions in Base such as map. A warning is issued if groupby is called with arguments in the old order.

Also, the GroupBy2 benchmark was not being run, and I assume that was omitted by mistake. GroupBy2 is now run.

[kmsquire]

The order argument makes sense to me, but I think the current version matches the ordering in DataFrames.

There is an issue open there to change the order there as well (JuliaData/DataFrames.jl#580), but it hasn't had much traffic recently. Might want to comment there.

While we wait for other comments, can you move the GroupBy2 benchmark fix to a separate PR?

[garborg]

Anything holding this up other than the already-merged test change needing to be removed from the PR?

FWIF, the referenced DataFrames method takes a Function as the first argument now (defined both ways).

[garborg]

Bump. Anyone anyone with write access have an opinion?

[kmsquire]

Makes sense to me, and the implementation looks reasonable at a glance. The merge conflict will have to be resolved.

[lendle]

I'll fix the merge conflicts later this week or weekend unless someone else
wants to.
On Jan 27, 2015 10:22 PM, "Kevin Squire" notifications@github.com wrote:

Makes sense to me, and the implementation looks reasonable at a glance.
The merge conflict will have to be resolved.

—
Reply to this email directly or view it on GitHub
#30 (comment).

[kmsquire]

Sounds good!

[garborg]

Great!

[coveralls]

Coverage decreased (-20.09%) to 73.48% when pulling 29509b6 on lendle:pull-request/95d755dd into a4a741c on JuliaLang:master.

[coveralls]

Coverage decreased (-20.09%) to 73.48% when pulling 6267fa7 on lendle:pull-request/95d755dd into a4a741c on JuliaLang:master.

[coveralls]

Coverage decreased (-20.09%) to 73.48% when pulling 981583b on lendle:pull-request/95d755dd into a4a741c on JuliaLang:master.

[coveralls]

Coverage decreased (-20.09%) to 73.48% when pulling 61cf20e on lendle:pull-request/95d755dd into a4a741c on JuliaLang:master.

[lendle]

I haven't been following julia issues closely lately but I think the travis failure for 0.4 is due to JuliaLang/julia#9947.

[garborg]

This passes locally for me on master and release-0.3. Anyone willing to either click the Restart Build button from within Travis or merge?

[simonster]

I restarted the build, but it failed again. It looks like the nightly hasn't been updated in 10 days because of JuliaLang/julia#10027.

[garborg]

Ah, I see. Thanks!

On Fri, Feb 13, 2015 at 11:04 AM, Simon Kornblith notifications@github.com
wrote:

Merged #30 #30.

—
Reply to this email directly or view it on GitHub
#30 (comment).