improve DataFrame constructors and conversions for Vector and Matrix #1325

bkamins · 2017-12-22T10:42:05Z

Following discussion in https://discourse.julialang.org/t/how-do-dataframes-jl-compare-to-rs-and-interoperability-between-r-and-julia/7387/14 I propose to:

Add a constructor of DataFrame from a Matrix that can take column names argument;
Add a method to convert allowing conversion from Vector to DataFrame (so that both the constructor and conversion work and work the same way);

nalimilan

Hmm... I'm not a big fan of these conversions between arrays and data frames, but since they already exist, we may as well support passing names.

Out of curiosity, how does it work in R? I couldn't find it.

nalimilan · 2017-12-22T17:12:07Z

src/dataframe/dataframe.jl

-    return DataFrame(cols, Index(gennames(n)))
-end
+Base.convert(::Type{DataFrame}, A::AbstractMatrix) = DataFrame(A)
+Base.convert(::Type{DataFrame}, V::AbstractVector) = DataFrame(V)


Note that this method is very different from the previous one as it expects the entries to be columns. I'm not sure we should support it, as it could be confusing compared with a 1-column matrix.

To say the truth I do not like DataFrame(::Vector) constructor, especially something like DataFrame([1,2,3]) is very non-intuivite. I pass a column vector and get a row of data.

But if we allow it then it is consistent to provide convert also.

Maybe a solution is to restrict the implementation of DataFrame(columns::AbstractVector to check internally:

all(col -> isa(col, AbstractVector), columns) || throw(ArgumentError("columns must be vectors"))

Yeah, it's a bit weird. Maybe we could also require specifying the column names when passing a vector, given that this is a rather low-level constructor.

agreed. this means that convert should be removed. I add deprecation to calling vector of vectors DataFrame without passing names.

nalimilan · 2017-12-22T17:12:32Z

src/dataframe/dataframe.jl

+    DataFrame(Any[columns[:, i] for i in 1:size(columns, 2)], cnames)
+end
+
+


One blank line is enough.

nalimilan · 2017-12-22T17:12:55Z

src/dataframe/dataframe.jl

@@ -125,6 +126,12 @@ function DataFrame(columns::AbstractVector,
    return DataFrame(convert(Vector{Any}, columns), Index(convert(Vector{Symbol}, cnames)))
 end

+# if #1308 is merged this sould be refactor to include makeunique
+function DataFrame(columns::AbstractMatrix, cnames::AbstractVector{Symbol} = gennames(size(columns, 2)))


Better use the short function syntax for one-liners like this.

bkamins · 2017-12-22T21:02:48Z

In R a matrix is actually our NamedArray - data.frame uses names of columns.
Actually this is something that could be considered do be added in NamedArrays.

bkamins · 2017-12-22T21:14:36Z

I will rebase and push this PR when CI works again

bkamins · 2017-12-23T23:21:02Z

I decided to recommend allowing DataFrames constructor (and thus convert) from AbstractVector only if it contains AbstractVector. This is breaking so I put deprecation for the time being.

Other issue is that tests on 0.7 fail (for different reasons - I am not sure if this is something to worry about, but in one case Julia crashed trying to precompile CategoricalArrays)

nalimilan · 2017-12-24T11:32:58Z

src/dataframe/dataframe.jl

@@ -122,9 +123,17 @@ end

 function DataFrame(columns::AbstractVector,
                   cnames::AbstractVector{Symbol} = gennames(length(columns)))
+    if !all(col -> isa(col, AbstractVector), columns)
+        # change to throw(ArgumentError("columns argument mutst be a vector of vectors"))


nalimilan · 2017-12-24T11:33:41Z

src/dataframe/dataframe.jl

    return DataFrame(convert(Vector{Any}, columns), Index(convert(Vector{Symbol}, cnames)))
 end

+# if #1308 is merged this sould be refactor to include makeunique


Let's merge #1308 first.

OK. I am pushing the as-is version to have it on GitHub, but I will rebase it when 1308 is merged.

nalimilan · 2017-12-24T11:40:01Z

src/dataframe/dataframe.jl

-    return DataFrame(cols, Index(gennames(n)))
-end
+Base.convert(::Type{DataFrame}, A::AbstractMatrix) = DataFrame(A)
+Base.convert(::Type{DataFrame}, V::AbstractVector) = DataFrame(V)


Yeah, it's a bit weird. Maybe we could also require specifying the column names when passing a vector, given that this is a rather low-level constructor.

nalimilan · 2017-12-26T15:32:22Z

src/dataframe/dataframe.jl

@@ -120,11 +121,23 @@ function DataFrame(; kwargs...)
    end
 end

-function DataFrame(columns::AbstractVector,
-                   cnames::AbstractVector{Symbol} = gennames(length(columns)))
+function DataFrame(columns::AbstractVector)


That function should be moved to deprecated.jl.

bkamins · 2017-12-29T22:01:27Z

rebased, added makeunique to Matrix constructor and in Vector constructor changed that it is required that at least one element is a vector (not all as was earlier).

nalimilan · 2017-12-30T17:01:23Z

I'd rather require all entries to be AbstractVector as you did originally, not just one of them. That's simpler, and since that's a low-level constructor is not a big deal to require the caller to create a vector.

bkamins · 2017-12-30T17:49:00Z

OK - fixed

nalimilan reviewed Dec 22, 2017

View reviewed changes

bkamins force-pushed the array_constructur branch from 02d66c3 to 33a7cc5 Compare December 23, 2017 22:59

bkamins mentioned this pull request Dec 24, 2017

Redesign getindex and fix stack/melt StackOverflow error #1305

Merged

nalimilan reviewed Dec 24, 2017

View reviewed changes

nalimilan reviewed Dec 26, 2017

View reviewed changes

improve DataFrame constructors from Matrix and Vector

59f2f92

bkamins force-pushed the array_constructur branch from 3ca91bd to 59f2f92 Compare December 29, 2017 21:58

require all columns to be vectors

690143d

Fix messages

49f1620

nalimilan approved these changes Dec 30, 2017

View reviewed changes

nalimilan merged commit 0fac3be into JuliaData:master Dec 30, 2017

ararslan mentioned this pull request Jan 16, 2018

How to mitigate deprecation warning when using orderby? JuliaData/DataFramesMeta.jl#82

Closed

bkamins deleted the array_constructur branch December 15, 2020 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve DataFrame constructors and conversions for Vector and Matrix #1325

improve DataFrame constructors and conversions for Vector and Matrix #1325

bkamins commented Dec 22, 2017

nalimilan left a comment

nalimilan Dec 22, 2017

bkamins Dec 22, 2017

nalimilan Dec 24, 2017

bkamins Dec 24, 2017

nalimilan Dec 22, 2017

bkamins Dec 22, 2017

nalimilan Dec 22, 2017

bkamins Dec 22, 2017

bkamins commented Dec 22, 2017

bkamins commented Dec 22, 2017

bkamins commented Dec 23, 2017

nalimilan Dec 24, 2017

bkamins Dec 24, 2017

nalimilan Dec 24, 2017

bkamins Dec 24, 2017

nalimilan Dec 24, 2017

nalimilan Dec 26, 2017

bkamins Dec 26, 2017

bkamins commented Dec 29, 2017

nalimilan commented Dec 30, 2017

bkamins commented Dec 30, 2017

		DataFrame(Any[columns[:, i] for i in 1:size(columns, 2)], cnames)
		end

improve DataFrame constructors and conversions for Vector and Matrix #1325

improve DataFrame constructors and conversions for Vector and Matrix #1325

Conversation

bkamins commented Dec 22, 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

bkamins commented Dec 22, 2017

bkamins commented Dec 22, 2017

bkamins commented Dec 23, 2017

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

bkamins commented Dec 29, 2017

nalimilan commented Dec 30, 2017

bkamins commented Dec 30, 2017