updates for 0.6 #73

bramtayl · 2017-07-04T06:34:36Z

Changes for 0.6, plus some very limited cleanups.

bramtayl · 2017-07-04T08:05:43Z

Well, I got 0.5 and 0.6 working; maybe I'll take another stab tomorrow on 0.7

nalimilan

Thanks! I can't really comment on the macro tricks, but it's great that tests pass on 0.6. I have a few mostly cosmetic comments.

nalimilan · 2017-07-04T18:04:45Z

src/DataFramesMeta.jl

-collect_ands(x::Expr) = x
-collect_ands(x::Expr, y::Expr) = :($x & $y)
-collect_ands(x::Expr, y...) = :($x & $(collect_ands(y...)))
+and(x, y) =


I'd say just drop support for versions before 0.6, anyway we already have a working version for these releases. @ararslan Does that sound OK to you?

Oh no! I spent all that time getting it working on 0.4 :(

Please switching this to the long function form.

nalimilan · 2017-07-04T18:05:46Z

src/DataFramesMeta.jl

@@ -43,27 +46,39 @@ replace_syms!(e::Expr, membernames) =

 protect_replace_syms!(e, membernames) = e
 protect_replace_syms!(e::Expr, membernames) =
-    e.head == :quote ? e : replace_syms!(e, membernames)
+    if e.head == :quote


The old version as a one-liner was more readable IMHO. If you change that you should use the long function syntax, but that doesn't sound like a good idea.

I like if else better than ternary because ternary gives artificially inflated coverage.

I wouldn't say that's a sufficient reason. If think it's been discussed in Base already.

Anyway, when you use if blocks, please also use the long function form (there are several cases like this), and keep the short syntax for one-liners.

nalimilan · 2017-07-04T18:05:54Z

src/DataFramesMeta.jl

@@ -28,6 +29,8 @@ onearg(e, f) = e.head == :call && length(e.args) == 2 && e.args[1] == f
 mapexpr(f, e) = Expr(e.head, map(f, e.args)...)

 replace_syms!(x, membernames) = x
+replace_syms!(q::QuoteNode, membernames) =
+    replace_syms!( Meta.quot(q.value), membernames)


Remove space.

nalimilan · 2017-07-04T18:06:03Z

src/DataFramesMeta.jl

@@ -1,6 +1,7 @@
 module DataFramesMeta

 using DataFrames
+import DataFrames


Why is this needed?

nalimilan · 2017-07-04T18:07:09Z

src/DataFramesMeta.jl

    end
 end

+function with_anonymous(body)
+    d = gensym()
+    anon( d, with_helper(d, body) )


Remove spaces too, that's not the style used in the file AFAICT.

nalimilan · 2017-07-04T18:14:04Z

src/compositedataframe.jl


 ### Arguments

 * `columns` : contains the contents of the columns
 * `cnames` : the names of the columns
 * `typename` : the optional name of the type created
 * `kwargs` : the key gives the column names, and the value is the column contents
+* `inmodule = DataFramesMeta` : a keyword argument to specify what module you
+want to define the type in. Consider passing `current_module()` or


Align this line and the next to `inmodule` AFAIK. If you drop support for Julia < 0.6, no need to mention old Julia versions.

current_module() is only deprecated as of 0.7.

OK. Would make sense to mention the versions then, since in a few months the old version won't be relevant at all.

nalimilan · 2017-07-04T18:16:38Z

test/compositedataframes.jl

+@test names(CompositeDataFrame(DataFrame(df), [:C, :D])) == [:C, :D]
+@test row() == nothing
+@test df[1, :A] == 1
+@test names( df[[1, 2]] ) == [:A, :B]


No spaces (here and below). [1, 2] could be 1:2.

I was trying hit coverage of the Base.getindex{T <: DataFrames.ColumnIndex}(cdf::AbstractCompositeDataFrame, col_inds::AbstractVector{T}) constructor

nalimilan · 2017-07-04T18:17:24Z

test/dataframes.jl

@@ -36,8 +37,8 @@ idx2 = :B
 @test  @where(df, :A .> x)          == df[df[:A] .> x,:]
 @test  @where(df, :B .> x)          == df[df[:B] .> x,:]
 @test  @where(df, :A .> :B)         == df[df[:A] .> df[:B],:]
-@test  @where(df, :A .> 1, :B .> 1) == df[(df[:A] .> 1) & (df[:B] .> 1),:]
-@test  @where(df, :A .> 1, :A .< 4, :B .> 1) == df[(df[:A] .> 1) & (df[:B] .> 1),:]
+@test  @where(df, :A .> 1, :B .> 1) == df[map(&, df[:A] .> 1, df[:B] .> 1),:]


If you drop support for Julia < 0.6, use .&.

nalimilan · 2017-07-04T18:17:44Z

test/dataframes.jl

    :colX = :B == 2 ? pi * :A : :B
    if :A > 1
        :colY = :A * :B
    end
 end
+
+import MacroTools


nalimilan · 2017-07-04T18:19:13Z

test/runtests.jl

@@ -1,4 +1,4 @@
-fatalerrors = false
+fatalerrors = true


Probably better keep false here, as it allows running all tests to see all errors, and only fail at the end.

nalimilan · 2017-07-04T20:38:13Z

src/compositedataframe.jl

-CompositeDataFrame(; kwargs...)
-CompositeDataFrame(typename::Symbol; kwargs...)
+CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}; inmodule = DataFramesMeta)
+CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}, typename::Symbol; inmodule = DataFramesMeta)


This line needs to be wrapped at 92 characters.

nalimilan · 2017-07-04T20:39:17Z

test/grouping.jl

@@ -14,9 +14,11 @@ g = groupby(d, :x)
 @test  DataFrame(@where(g, length(:x) > 5))[:n][1:3] == @data [5, 6, 7]

 @test  DataFrame(orderby(g, x -> mean(x[:n]))) == DataFrame(@orderby(g, mean(:n)))
-#@test  DataFrames.based_on(@orderby(g, mean(:n)), x -> x[1,:x])[:x1] == [3,1,2]


Why remove this? Is it tested elsewhere? Else, better keep it as it might reflect a feature which is supposed to work but doesn't.

I added in a @based_on test which should cover this.

bramtayl · 2017-07-05T20:59:54Z

Got to pass on 0.7. Looks like @> from Lazy is broken.

nalimilan

Great. I have a few remaining comments about coding style then it should be good to go (if @ararslan wants to merge after that...).

nalimilan · 2017-07-05T21:06:37Z

src/DataFramesMeta.jl

+0×2 DataFrames.DataFrame
+
+julia> d = DataFrame(
+            n = 1:20,


That's an unusual way of wrapping lines. I'd rather keep n on the first line, but split x over multiple lines. Same remark in the next docstring.

nalimilan · 2017-07-05T21:07:07Z

src/DataFramesMeta.jl

-collect_ands(x::Expr) = x
-collect_ands(x::Expr, y::Expr) = :($x & $y)
-collect_ands(x::Expr, y...) = :($x & $(collect_ands(y...)))
+and(x, y) =


Please switching this to the long function form.

nalimilan · 2017-07-05T21:08:01Z

src/DataFramesMeta.jl

-        end
-    end
+    _D = gensym()
+    :(let $_D = $d


Why not keep quote... end? Looks cleaner.

nalimilan · 2017-07-05T21:08:51Z

src/DataFramesMeta.jl

+
+julia> g = groupby(d, :x);
+
+julia> @macroexpand @orderby(g, mean(:n))


What does @macroexpand do here?

nalimilan · 2017-07-05T21:09:20Z

src/DataFramesMeta.jl

-    newargs = [args...]
-    map!(convert_kw!, newargs)
-    :( transform($x, $(newargs...)) )
+    :( $transform($x, $(map(args) do kw


quote... end could make sense here too.

nalimilan · 2017-07-05T21:13:38Z

src/byrow.jl

+julia> df = DataFrame(A = 1:3, B = [2, 1, 2]);
+
+julia> let x = 0
+            @byrow!(df, if :A + :B == 3; x += 1 end)  #  This doesn't work without the let


Would be clearer with begin... end block on multiple lines. Same below.

nalimilan · 2017-07-05T21:14:06Z

src/byrow.jl

+│ 3   │ 0 │ 2 │
+
+julia> df2 = @byrow! df begin
+            @newcol colX::Array{Float64}


Four-space indentation.

nalimilan · 2017-07-05T21:15:03Z

src/compositedataframe.jl

-CompositeDataFrame(; kwargs...)
-CompositeDataFrame(typename::Symbol; kwargs...)
-```
+    CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}; inmodule = DataFramesMeta)


Wrap lines at 92 chars.

nalimilan · 2017-07-05T21:16:01Z

src/linqmacro.jl

+julia> n = 100;
+
+julia> df = DataFrame(a = rand(1:3, n),
+            b = ["a","b","c","d"][rand(1:4, n)],


Align with a.

nalimilan · 2017-07-05T21:16:25Z

src/linqmacro.jl

+│ 3   │ "d" │ 0.568289 │ 5.68289 │
+
+julia> @linq df |>
+            transform(y = 10 * :x) |>


Four-space indent.

nalimilan · 2017-07-05T21:19:40Z

.travis.yml

@@ -1,7 +1,7 @@
 language: julia
 julia:
-  - 0.4


Also remove the README.md badge.

nalimilan

Looks good, though there's still a small an alignment issue that would be nice to fix. Thanks!

@ararslan OK to merge?

nalimilan · 2017-07-07T22:00:27Z

src/compositedataframe.jl

@@ -38,7 +38,8 @@ row() = nothing

 """
    CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}; inmodule = DataFramesMeta)
-    CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}, typename::Symbol; inmodule = DataFramesMeta)
+    CompositeDataFrame(columns::Vector{Any}, cnames::Vector{Symbol}, typename::Symbol;
+        inmodule = DataFramesMeta)


Should be aligned with column.

nalimilan · 2017-07-07T22:03:36Z

Hmm, and you should remove Julia 0.4. from appveyor.yml, which will make the CI turn green.

ararslan · 2017-07-07T23:21:39Z

src/DataFramesMeta.jl

+    else
+        :($x .& $y)
+    end
+end


It would be better to move the conditional outside of the function definition, i.e.

if VERSION < v"0.6.0-" and(x, y) = :($x & $y) else and(x, y) = :($x .& $y) end

Otherwise the version check is performed every time the function is called.

ararslan · 2017-07-07T23:25:11Z

src/byrow.jl

@@ -28,18 +28,22 @@ byrow_replace(x) = x

 function byrow_find_newcols(e::Expr, newcol_decl)
    if e.head == :macrocall && e.args[1] == Symbol("@newcol")
-        ea = e.args[2]
+        if VERSION < v"0.7-"


It would be good to be more exact with the version cutoff used. What changed in Base such that the number of arguments in the expression increased? If you can find the PR that introduced that change, you can use contrib/commit-name.sh in the Julia repo to get the exact version that corresponds to that change.

Also since this is in a function (but in this case the conditional is less easily separable), the condition should have an @static to evaluate the branch once at parse time.

Probably JuliaLang/julia@3c3ced4? That's 0.7.0-DEV.481.

Actually I think it was this: JuliaLang/julia#21746

Try :(@newcol Array{Int}).args in 0.7

That would be 0.7.0-DEV.357 then.

nalimilan · 2017-07-11T08:00:40Z

Can you also remove Julia 0.4 from appveyor.yml?

nalimilan · 2017-07-11T16:50:25Z

appveyor.yml

  - JULIA_URL: "https://julialang-s3.julialang.org/bin/winnt/x86/0.5/julia-0.5-latest-win32.exe"
  - JULIA_URL: "https://julialang-s3.julialang.org/bin/winnt/x64/0.5/julia-0.5-latest-win64.exe"
+  - JULIA_URL: "https://julialang-s3.julialang.org/bin/winnt/x86/0.6/julia-0.5-latest-win32.exe"


Typo 0.5 in the filename... :-/

nalimilan · 2017-07-11T20:20:58Z

Thanks! It's really cool to have DataFramesMeta working on Julia 0.6.

bramtayl · 2017-07-11T20:38:35Z

Yeah for sure. I think there's a number of improvements possible going forward. For example, it seems easy enough to widen the scope to include datatables as well. Also, I wonder if anything could be done about type stability/other optimizations for the grouped operations. I've been consider adding DataFramesMeta as a (second) backend to LazyQuery. For some things, like grouped operations, DataFramesMeta seems much more intuitive than Query (which currently doesn't have a good alternative to dplyr summarize).

tkelman · 2017-07-12T09:28:46Z

src/compositedataframe.jl

@@ -9,9 +9,9 @@ export AbstractCompositeDataFrame, AbstractCompositeDataFrameRow,
 An abstract type that is an `AbstractDataFrame`. Each type that inherits from
 this is expected to be a type-stable data frame.
 """
-abstract AbstractCompositeDataFrame <: AbstractDataFrame
+@compat abstract type AbstractCompositeDataFrame <: AbstractDataFrame end


this requires Compat 0.17

bramtayl added 5 commits July 4, 2017 02:25

updates for 0.6

6bbcfbc

get 0.5 on board (hopefully)

5cfdd1e

fixed some depwarns

6453883

no structs

72c98fb

bugfixes

cd95bd0

joshbode mentioned this pull request Jul 4, 2017

fix some deprecation warnings and errors for 0.6 #72

Closed

bramtayl added 5 commits July 4, 2017 11:05

no dot broadcasting

abcfb6b

newcol fixes

d0d32c1

should fix 0.4

502cb6e

much more coverage

a52d908

even more coverage

5968b6b

nalimilan reviewed Jul 4, 2017

View reviewed changes

bramtayl added 3 commits July 4, 2017 15:19

added some comments

028ff68

missed one

cf55db6

drop support for 0.4

083f157

nalimilan reviewed Jul 4, 2017

View reviewed changes

nalimilan mentioned this pull request Jul 5, 2017

Missing DataArray{T}(dims) constructors JuliaStats/DataArrays.jl#271

Open

doctests, fix for 0.7, quote blocks (less coverage)

23b106a

nalimilan reviewed Jul 5, 2017

View reviewed changes

.travis.yml

@@ -1,7 +1,7 @@

language: julia

julia:

- 0.4

Copy link

Member

nalimilan Jul 5, 2017

Choose a reason for hiding this comment

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

Also remove the README.md badge.

addressed comments

f23196b

nalimilan approved these changes Jul 7, 2017

View reviewed changes

ararslan reviewed Jul 7, 2017

View reviewed changes

addressed more comments

f9281b4

remove 0.4

103582e

nalimilan reviewed Jul 11, 2017

View reviewed changes

fixed typo

5904300

nalimilan merged commit c6a52e3 into JuliaData:master Jul 11, 2017

tkelman reviewed Jul 12, 2017

View reviewed changes

tshort mentioned this pull request Jan 30, 2018

Support _DF (or something) for full DataFrame access #86

Closed


		julia> g = groupby(d, :x);

		julia> @macroexpand @orderby(g, mean(:n))

updates for 0.6 #73

updates for 0.6 #73

Conversation

bramtayl commented Jul 4, 2017

bramtayl commented Jul 4, 2017

nalimilan left a comment

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

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

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

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

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

Choose a reason for hiding this comment

bramtayl commented Jul 5, 2017

nalimilan left a comment

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

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

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

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan Jul 7, 2017 • edited

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

Choose a reason for hiding this comment

nalimilan commented Jul 11, 2017

Choose a reason for hiding this comment

nalimilan commented Jul 11, 2017

bramtayl commented Jul 11, 2017

Choose a reason for hiding this comment

ararslan Jul 7, 2017 •

edited