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

updates for 0.6 #73

Merged
merged 18 commits into from
Jul 11, 2017
Merged

updates for 0.6 #73

merged 18 commits into from
Jul 11, 2017

Conversation

bramtayl
Copy link
Contributor

@bramtayl bramtayl commented Jul 4, 2017

Changes for 0.6, plus some very limited cleanups.

@bramtayl
Copy link
Contributor Author

bramtayl commented Jul 4, 2017

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

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.

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.

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) =
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Please switching this to the long function form.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Remove space.

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

using DataFrames
import DataFrames
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

end
end

function with_anonymous(body)
d = gensym()
anon( d, with_helper(d, body) )
Copy link
Member

Choose a reason for hiding this comment

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

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


### 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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current_module() is only deprecated as of 0.7.

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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),:]
Copy link
Member

Choose a reason for hiding this comment

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

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

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

import MacroTools
Copy link
Member

Choose a reason for hiding this comment

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

?

test/runtests.jl Outdated
@@ -1,4 +1,4 @@
fatalerrors = false
fatalerrors = true
Copy link
Member

Choose a reason for hiding this comment

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

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

This line needs to be wrapped at 92 characters.

@@ -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]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@bramtayl
Copy link
Contributor Author

bramtayl commented Jul 5, 2017

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

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.

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

0×2 DataFrames.DataFrame

julia> d = DataFrame(
n = 1:20,
Copy link
Member

Choose a reason for hiding this comment

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

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.

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) =
Copy link
Member

Choose a reason for hiding this comment

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

Please switching this to the long function form.

end
end
_D = gensym()
:(let $_D = $d
Copy link
Member

Choose a reason for hiding this comment

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

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


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

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

Choose a reason for hiding this comment

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

What does @macroexpand do here?

newargs = [args...]
map!(convert_kw!, newargs)
:( transform($x, $(newargs...)) )
:( $transform($x, $(map(args) do kw
Copy link
Member

Choose a reason for hiding this comment

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

quote... end could make sense here too.

src/byrow.jl Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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

src/byrow.jl Outdated
│ 3 │ 0 │ 2 │

julia> df2 = @byrow! df begin
@newcol colX::Array{Float64}
Copy link
Member

Choose a reason for hiding this comment

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

Four-space indentation.

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

Choose a reason for hiding this comment

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

Wrap lines at 92 chars.

src/linqmacro.jl Outdated
julia> n = 100;

julia> df = DataFrame(a = rand(1:3, n),
b = ["a","b","c","d"][rand(1:4, n)],
Copy link
Member

Choose a reason for hiding this comment

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

Align with a.

src/linqmacro.jl Outdated
│ 3 │ "d" │ 0.568289 │ 5.68289 │

julia> @linq df |>
transform(y = 10 * :x) |>
Copy link
Member

Choose a reason for hiding this comment

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

Four-space indent.

@@ -1,7 +1,7 @@
language: julia
julia:
- 0.4
Copy link
Member

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.

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.

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

@ararslan OK to merge?

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Should be aligned with column.

@nalimilan
Copy link
Member

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

else
:($x .& $y)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

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.

src/byrow.jl Outdated
@@ -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-"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ararslan ararslan Jul 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

That would be 0.7.0-DEV.357 then.

@nalimilan
Copy link
Member

Can you also remove Julia 0.4 from appveyor.yml?

appveyor.yml Outdated
- 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"
Copy link
Member

Choose a reason for hiding this comment

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

Typo 0.5 in the filename... :-/

@nalimilan nalimilan merged commit c6a52e3 into JuliaData:master Jul 11, 2017
@nalimilan
Copy link
Member

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

@bramtayl
Copy link
Contributor Author

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).

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

this requires Compat 0.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants