Small no-op refactor before functionality change #2681

sa- · 2021-03-26T21:45:22Z

In preparation for #2423 I'm making a no-op refactor.

I would feel comfortable going through the process of contributing with a small no-op change like this one before making any functionality changes.

bkamins · 2021-03-26T23:22:04Z

src/groupeddataframe/splitapplycombine.jl

                                        ColumnIndex, MultiColumnIndex}...);
                          keepkeys::Bool, ungroup::Bool, copycols::Bool,
                          keeprows::Bool, renamecols::Bool)
    if !ungroup && !keepkeys
        throw(ArgumentError("keepkeys=false when ungroup=false is not allowed"))
    end

-    cs_vec = []
-    for p in cs
+    processed_combine_ops = map(combine_operations) do p


we do not want to use map here to avoid excessive specialization. Please leave the Vector{Any} as a container for processed_combine_ops.

bkamins · 2021-03-26T23:23:37Z

src/groupeddataframe/splitapplycombine.jl

@@ -65,7 +62,9 @@ function _combine_prepare(gd::GroupedDataFrame,

    idx, valscat = _combine(gd, cs_norm, optional_transform, copycols, keeprows, renamecols)

-    !keepkeys && ungroup && return valscat
+    if !keepkeys && ungroup


I think the previous code was idiomatic Julia style. Why do you want to change it?

Ah okay, good to know

bkamins · 2021-03-26T23:24:18Z

Project.toml

@@ -40,6 +40,7 @@ julia = "1"

 [extras]
 CategoricalArrays = "324d7699-5711-5eae-9e2f-1d82baa6b597"
+Chain = "8be319e6-bccf-4806-a6f7-6fae938471bc"


I do not see it used in this PR

bkamins · 2021-03-26T23:26:54Z

Also looking by the CI errors you have changed the logic somewhere (I did not dig into it as I understand this is still a draft).

bkamins · 2021-03-26T23:30:02Z

src/groupeddataframe/splitapplycombine.jl

        else
-            push!(cs_vec, p)
+            p


this line is incorrect as you mix vector and scalar values in it. Note the distinction between append! and push! in the original code.

I noticed that.. from what I read the functionality looked similar. How do the functions work differently under the hood?

bkamins · 2021-03-26T23:33:34Z

In summary: thank you very much for working on it. I think that the refactorizations that improve code readability are very useful and we should go for them.

However, I would recommend you to start with refactorizations that do not change the logic of the code (like here you tried to change for loop to map and this change has a significant impact on the code logic).

Of course I am not opposed to such changes in general in the future - if they would improve things, but they are much more tricky than just no-op refactorizations, so it is probably better to do them when you get more experience with the codebase.

bkamins · 2021-03-26T23:38:19Z

Also - in general - especially to start with - it is best if you push PR that pass tests locally with:

]test DataFrames

In the future you will notice that this is not a strict rule as sometimes one pushes a PR that fails to discuss some design ideas, but for start I think it is useful to build a habit that commits are pushed to GitHub when they pass tests locally.

sa-

Hey, I wasn't expecting anyone to look at this but I appreciate that you did anyway!

Some thoughts -

Makes sense that the refactoring would be a good idea after I'm comfortable with the codebase
I was hoping that my refactoring changes would not make any functional changes but that seems harder to do than I initially thought e.g. I didn't think changing for to map would make a difference but I've clearly broken something.

sa- · 2021-03-27T07:41:18Z

src/groupeddataframe/splitapplycombine.jl

        else
-            push!(cs_vec, p)
+            p


I noticed that.. from what I read the functionality looked similar. How do the functions work differently under the hood?

sa- · 2021-03-27T07:42:18Z

src/groupeddataframe/splitapplycombine.jl

@@ -65,7 +62,9 @@ function _combine_prepare(gd::GroupedDataFrame,

    idx, valscat = _combine(gd, cs_norm, optional_transform, copycols, keeprows, renamecols)

-    !keepkeys && ungroup && return valscat
+    if !keepkeys && ungroup


Ah okay, good to know

bkamins · 2021-03-27T08:43:24Z

I didn't think changing for to map would make a difference but I've clearly broken something.

Here is an example. Current code:

julia> cs = (nrow, [1,2], 3, [1 => 4,  2 => 5])
(DataFrames.nrow, [1, 2], 3, [1 => 4, 2 => 5])

julia> vec_cs = Any[]
Any[]

julia> for p in cs
           if p === nrow
               push!(vec_cs, nrow => :nrow)
           elseif p isa AbstractVecOrMat{<:Pair}
               append!(vec_cs, p)
           else
               push!(vec_cs, p)
           end
       end

julia> vec_cs
5-element Vector{Any}:
 DataFrames.nrow => :nrow
                  [1, 2]
                 3
               1 => 4
               2 => 5

your code:

julia> map(cs) do p
           if p === nrow
               nrow => :nrow
           else
               p
           end
       end
(DataFrames.nrow => :nrow, [1, 2], 3, [1 => 4, 2 => 5])

So you can see that:

type of the return value changed
structure of the collection changed

A more advanced comment was about specialization. Methods for Vector{Any} are precompiled in Julia Base, so they do not trigger specialization. If you use map a new Tuple is created. As Tuple has a type that depends on all its elements the whole function has to be recompiled on each call, while the original function had to be compiled only once (on the first call).

I wasn't expecting anyone to look at this but I appreciate that you did anyway!

I assumed you wanted to get some feedback to learn a bit about the design concepts behind DataFrames.jl internals.

sa- added 4 commits March 26, 2021 22:43

small no-op refactor before code change

b92aa09

no ws in toml files

265e02d

whitespace was fine, there was an unclosed comma

3dd97bf

getting the variable naming right

b8b4cbb

bkamins reviewed Mar 26, 2021

View reviewed changes

bkamins added the intro issue label Mar 26, 2021

bkamins added this to the 1.x milestone Mar 26, 2021

sa- commented Mar 27, 2021

View reviewed changes

sa- closed this Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small no-op refactor before functionality change #2681

Small no-op refactor before functionality change #2681

sa- commented Mar 26, 2021

bkamins Mar 26, 2021

bkamins Mar 26, 2021

sa- Mar 27, 2021

bkamins Mar 26, 2021

bkamins commented Mar 26, 2021

bkamins Mar 26, 2021

sa- Mar 27, 2021

bkamins commented Mar 26, 2021

bkamins commented Mar 26, 2021

sa- left a comment

sa- Mar 27, 2021

sa- Mar 27, 2021

bkamins commented Mar 27, 2021

Small no-op refactor before functionality change #2681

Small no-op refactor before functionality change #2681

Conversation

sa- commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Mar 26, 2021

bkamins commented Mar 26, 2021

sa- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Mar 27, 2021