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

collecting with brackets is deprecated #939

Merged

Conversation

gustafsson
Copy link
Contributor

Resolves WARNING: [a] concatenation is deprecated; use collect(a) instead

Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
@nalimilan
Copy link
Member

Thanks. The tests failed on Julia nightlies because the name of the column now depends on that of lambda function (which cannot be predicted reliably). This can be fixed by indexing the data frames with [1] before comparing them. But are you sure you need a new test? This line seems to be covered already (deprecations do not cause failures): https://coveralls.io/builds/5604859/source?filename=src%2Fgroupeddataframe%2Fgrouping.jl#L248

Also, I wonder whether we actually need to call collect here. Some functions could return e.g. generators, and it could be useful. I don't see why we should impose returning an array.

@tshort Opinions?

@tshort
Copy link
Contributor

tshort commented Apr 7, 2016

I'm not where I can test anything, but I think you will need collect. If f is a reduction function, collect will convert it to an array. If f is an element-wise function, it will keep the result as an array.

@nalimilan
Copy link
Member

@tshort Yes, but do we really need an array? Since the result is an Array{Any}, having some elements be scalars (or even ranges or generators) isn't an issue.

@gustafsson
Copy link
Contributor Author

I replaced the lambda function with a regular element-wise function in the added commit above.

I think all other tests of colwise uses reduction functions because I don't see this warning without the test I added.

The elements of colwise should be of type AbstractVector according to test/grouping.jl. Perhaps this could be relaxed?

Would it make sense to use something like this instead of collect? (it doesn't make a difference for generators but it would save an unnecessary copy of arrays)

makeabstractvector(x::AbstractVector) = x
makeabstractvector(x) = collect(x)

Without using collect I get these errors (julia homebrew Version 0.4.5 (2016-03-18 00:58 UTC)):

$ cd ~/.julia/v0.4/DataFrames/test
$ julia runtests.jl

...

    FAILED: data.jl
LoadError: MethodError: `_isnullable` has no method matching _isnullable(::Int64)
 [inlined code] from ~/.julia/v0.4/DataFrames/test/runtests.jl:44
 in anonymous at no file:0
 in include at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in include_from_node1 at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in process_options at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in _start at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
while loading ~/.julia/v0.4/DataFrames/test/data.jl, in expression starting on line 107

...

    FAILED: grouping.jl
LoadError: test failed: all((T->begin  # ~/.julia/v0.4/DataFrames/test/grouping.jl, line 20:
            T <: AbstractVector
        end),map(typeof,colwise([sum],df)))
 in expression: all((T->begin  # ~/.julia/v0.4/DataFrames/test/grouping.jl, line 20:
            T <: AbstractVector
        end),map(typeof,colwise([sum],df)))
 [inlined code] from ~/.julia/v0.4/DataFrames/test/runtests.jl:44
 in anonymous at no file:0
 in include at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in include_from_node1 at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in process_options at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
 in _start at /usr/local/Cellar/julia/0.4.5/lib/julia/sys.dylib
while loading ~/.julia/v0.4/DataFrames/test/grouping.jl, in expression starting on line 20

@gustafsson
Copy link
Contributor Author

@nalimilan aggregate(DataFrame(a=1),abs) still gives the same warning on master. It could be worth nothing that in the documentation for aggregate it says:

Each fs should return a value or vector. All returns must be the same length.

The problem addressed by this PR seems to only arise with functions that returns vectors (such as abs). So for instance aggregate(DataFrame(a=1),sum) isn't a problem in master.

@nalimilan
Copy link
Member

Let's go with this as it fixes the warning. We can always relax the requirements later, but that's kind of a separate issue. sorry it took so long (do not hesitate to bump from time to time).

@nalimilan nalimilan merged commit 5998148 into JuliaData:master Sep 27, 2016
@nalimilan
Copy link
Member

Woops, this actually made the tests fail after the port the Nullable. Could you check that #1075 is correct?

@gustafsson gustafsson deleted the colwise_collect_without_brackets branch September 27, 2016 11:09
maximerischard pushed a commit to maximerischard/DataFrames.jl that referenced this pull request Sep 28, 2016
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
nalimilan pushed a commit that referenced this pull request Jul 8, 2017
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
nalimilan pushed a commit that referenced this pull request Jul 8, 2017
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
rofinn pushed a commit that referenced this pull request Aug 17, 2017
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
nalimilan pushed a commit that referenced this pull request Aug 25, 2017
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
quinnj pushed a commit that referenced this pull request Sep 2, 2017
Resolve "WARNING: [a] concatenation is deprecated; use collect(a) instead"
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.

3 participants