[BREAKING] deprecate DataFrame constructors #2464

bkamins · 2020-10-01T11:17:15Z

Fixes #2433. Before I change tests and documentation please review if we agree with what is proposed to get deprecated. Thank you.

src/dataframe/dataframe.jl

src/deprecated.jl

src/dataframe/dataframe.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

matthieugomez · 2020-10-03T19:50:44Z

Yes, my view is that it’s best to reduce the number of methods before 1.0. It can always be added back later.

…

On Sat, Oct 3, 2020 at 12:43 PM Bogumił Kamiński ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/deprecated.jl <#2464 (comment)> : > + makeunique=makeunique, copycols=copycols) ***@***.*** DataFrame(columns::NTuple{N, AbstractVector}, cnames::NTuple{N, AbstractString}; makeunique::Bool=false, + copycols::Bool=true) where {N} DataFrame(collect(AbstractVector, columns), [Symbol(c) for c in cnames]; + makeunique=makeunique, copycols=copycols) ***@***.*** DataFrame(columns::NTuple{N, AbstractVector}; + copycols::Bool=true) where {N} DataFrame(collect(AbstractVector, columns), + gennames(length(columns)), copycols=copycols) ***@***.*** DataFrame(columns::AbstractMatrix, cnames::AbstractVector{Symbol} = gennames(size(columns, 2)); + makeunique::Bool=false) DataFrame(AbstractVector[columns[:, i] for i in 1:size(columns, 2)], + cnames; makeunique=makeunique, copycols=false) + ***@***.*** DataFrame(columns::AbstractMatrix, cnames::AbstractVector{<:AbstractString}; + makeunique::Bool=false) DataFrame(AbstractVector[columns[:, i] for i in 1:size(columns, 2)], + Symbol.(cnames); makeunique=makeunique, copycols=false) + +function DataFrame(column_eltypes::AbstractVector{T}, cnames::AbstractVector{Symbol}, @matthieugomez <https://github.com/matthieugomez> - is 👍 for dropping it? (just to be sure as I tried to be neutral with the question) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2464 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPPPXLHHIREW3HPOSPINKTSI55HFANCNFSM4SAJGAXQ> .

bkamins · 2020-10-06T08:14:19Z

OK - so the decision is (unless I grossly misunderstood something 😄): we keep the current proposal (including deprecating eltype constructors).

I will wait for 1-2 days in case more comments come in and then update the whole package not to use deprecated functionality (mainly tests).

bkamins · 2020-10-08T12:41:40Z

It should be good for a review.

bkamins · 2020-10-08T18:25:42Z

@rofinn - can you please check if with this PR your use case works as expected?

rofinn · 2020-10-08T22:08:22Z

Yep, looks good to me. I only really use the tables or pairs constructors anyways. 👍

bkamins · 2020-10-09T19:13:32Z

Last commit follows the discussion in JuliaData/Tables.jl#204 and fixes embarrassing cases we had:

julia> DataFrame(Pair[])
1×1 DataFrame
│ Row │ makeunique │
│     │ Bool       │
├─────┼────────────┤
│ 1   │ 0          │

julia> DataFrame(Any["a" => [1]])
1×2 DataFrame
│ Row │ first  │ second │
│     │ String │ Array… │
├─────┼────────┼────────┤
│ 1   │ a      │ [1]    │

bkamins · 2020-10-11T16:47:54Z

This should be good for a final check before merging.

src/dataframe/dataframe.jl

src/deprecated.jl

src/dataframe/dataframe.jl

nalimilan · 2020-10-12T20:41:39Z

src/other/tables.jl

-        if x isa AbstractVector && all(col -> isa(col, AbstractVector), x)
+    if !Tables.istable(x) && x isa AbstractVector && !isempty(x)
+        # here we handle the cases that are accepted by standard DataFrame constructors
+        # but eltype(x) is more flexible than assumed in these methods


I don't really understand this comment. Do you mean that here we handle cases where eltype(x) doesn't match what other (more standard) constructors accept?

Exactly. What we handle is essentially Any[...] case (or some weird Union{...}[...]) as it will not be handled by standard constructors.

It was not very clear earlier and this is essentially what this code path is for only.

OK. I think what confused me is "that are accepted by standard DataFrame constructors", since these are not really accepted, or they wouldn't have to be dealt with in this method. :-)

Can you - as usual - propose a better wording (or what we have now is OK?). Thank you!

nalimilan · 2020-10-13T10:30:14Z

test/broadcasting.jl

        @test (df .+ df) ./ 2 !== df
        @test df .+ Matrix(df) == 2 .* df
        @test Matrix(df) .+ df == 2 .* df
-        @test (Matrix(df) .+ df .== 2 .* df) == DataFrame(trues(size(df)), names(df))
+        @test (Matrix(df) .+ df .== 2 .* df) == DataFrame(Tables.table(trues(size(df)), header=names(df)))


Looks at these examples, it feels unfortunate to lose the symmetry between Matrix(df) and DataFrame(mat), especially given the fact that broadcasting between data frames and matrices is allowed.

I think it makes sense to have some asymmetry, since Matrix(df) can just drop names, but DataFrame(mat) needs to generate names: so requiring DataFrame(mat, colnames) isn't illegitimate. But DataFrame(Tables.table(df, header=colnames)) really breaks that symmetry (and it's really verbose as we see here). Maybe we could keep DataFrame(mat, colnames), since in that case there's no ambiguity (mat is treated as a matrix, not as a Table.jl object).

(Sorry for suggesting this now. I can adjust the tests if you like.)

I am OK with this (I can fix the tests - it should be quick this way as we have Tables.table in all places we need to fix 😄).

@oxinabox + @pdeffebach : can you comment here please?

The proposal is to have DataFrame(::AbstractMatrix, ::Union{AbstractVector{Symbol}, AbstractVector{<:AbstractString}) signature (with no default for the second positional argument, so it would be required).

Then there would be no ambiguity with DataFrame(table) constructor that can handle matrices as tables.

Once again, I stand by my claim that every extra constructor adds confusion.
For this case i think nicer than using Tables.jl is:

DataFrame(names .=> eachcol(mat))

that is a nice clear explict statement about what is going on, and is I think clearer than
DataFrame(mat, names) since we are explictly stating that we are going to use the columns one per name.
Also naturally extends to:
DataFrame(names .=> vec.(eachrow(mat))
for constructing dataframes based on the rows of the matrix.
Particularly relevent as sometimes we do observations as columns, e.g. Distances.jl, the Models.jl interface, some others.

Constructing DataFrames from a matrix is in general a weird thing to do outside of toy examples
because a typical dataframe has a mixture of different types of columns,
and a typical matrix does not.
I don't think we should go out of our way to encourage people writing unrealistic toy examples, by making it easier

@nalimilan - @oxinabox has convinced me. Writing DataFrame(names .=> eachcol(mat)) is indeed clean. We have to use Tables.table for now because eachcol is not supported in Julia 1.0, but once we move the support to new Julia LTS we can clean it up.

What I find problematic is that by supporting Matrix(df) and df .+ mat, we consider that data frames and matrices are objects with compatible shapes. This alone means that we exclude the view of observations as columns (which BTW is the exception in the ecosystem, even in Distances you now have to explicitly give dims). Yet DataFrame(names .=> eachcol(mat)) conceptually requires you to decompose the matrix into columns to create a data frame. The notion that shapes are compatible is completely lost.

* `DataFrame` constructor is about being a table (which in particular implies having column names)

I don't think this rule holds in general, does it? It's only true for the one-argument constructor (and even in that case, not e.g. for the ::GroupedDataFrame one). For the two-argument constructor, we don't actually allow tables as the first argument. So it would be OK to accept matrices (just like we accept vectors of vectors).

I don't think this rule holds in general, does it?

We have to have some minimal set of constructors. For instance I hate DataFrame(a=[1,2], b=[2,3]) constructor as it is inconsistent with the general design we have, but I think we should keep it as it is convenient. How I understand @oxinabox is that he wants to limit the number of special cases as much as possible, and it seems that for matrices DataFrame(names .=> eachcol(matrix)) is an easy enough pattern.

For instance I hate DataFrame(a=[1,2], b=[2,3]) constructor as it is inconsistent with the general design we have

I was going to suggest deprecating that as well; for DataFrame(:a=>[1,2], ...) 😂
because getting rid of it would mean we are free to add what ever keywords we wanted to the constructor later without clashing with column names.

we have this clash already with copycols and makeunique kwargs which are handled inconsistently:

julia> DataFrame(makeunique=2, copycols=false) 1×1 DataFrame │ Row │ makeunique │ │ │ Int64 │ ├─────┼────────────┤ │ 1 │ 2 │

I was going to suggest deprecating that as well; for DataFrame(:a=>[1,2], ...)

Oh, is that an option here? I'm 100% on board with that change 😸

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

…recate_constructors

nalimilan

Thanks! Looks mostly good I'd say.

src/dataframe/dataframe.jl

test/broadcasting.jl

src/other/tables.jl

test/broadcasting.jl

src/dataframe/dataframe.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2020-11-03T10:51:32Z

I have applied all the suggestions. It should be good for another look at.

bkamins · 2020-11-03T15:38:38Z

CI passes cleanly (I am testing against current master with all the changes made since this PR was started)

src/dataframe/dataframe.jl

nalimilan · 2020-11-05T21:43:11Z

test/constructors.jl

@@ -337,4 +344,11 @@ end
    end
 end

+@testset "Dict constructor corner case" begin
+    @test DataFrame(Dict('a' => 1, true => 2)) == DataFrame("a" => 1, "true" => 2)


Should we really allow this?

This was the original design. Actually I thought that maybe it is best to allow only string or Symbol as keys. It is last time to change it :). Do we go this way?

That sounds safer -- as usual. :-)

OK - I will change this then. Thank you!

done - now we are as strict as with Pair constructors (except for sorting when Dict is passed).

bkamins · 2020-11-06T16:35:32Z

CI passes, only coverage fails

src/dataframe/dataframe.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2020-11-06T16:47:40Z

Thank you! I will merge this once the CI passes.

src/dataframe/dataframe.jl

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2020-11-06T21:55:52Z

Thank you!

0kto · 2020-11-16T13:45:09Z

Hi all, I am late to this discussion.
I updated yesterday, and with v0.22 I receive the deprecation warning:

DataFrame` constructor with passed eltypes is deprecated.
[...]

Reading through the docs and the source code, I was not able to find a suitable replacement:
In my scripts I initialize a DataFrame object by defining column-names and types (and zero rows), to which I append data read from (rather messy) instrument data files. Having the eltypes defined before appending data to the DataFrame, bad data (where the Type is not matching) fails and immediately helps figuring out if there is a problem. Also, adding a column is as easy as adding a Dictionary entry, making my code easily usable for non-developers.

So, my question is if there is a suitable replacement I haven't seen, or if there is a recommended workflow I could implement instead. Or if you could revive at least one constructor that accepts a Type declaration for columns.

pdeffebach · 2020-11-16T14:13:53Z

How about this? I think it's decently intuitive

julia> d = Dict(:id => String, :income => Float64, :education => Int64);

julia> df = DataFrame([n => T[] for (n, T) in d])
0×3 DataFrame

bkamins · 2020-11-16T14:45:17Z

or just e.g.:

DataFrame([:a, :b] .=> [T[] for T in [Int, Float64]])

Dict is not 100% correct as it will reorder columns.

0kto · 2020-11-16T17:24:10Z

Thanks, that is really helpful.
Since I am using an OrderedDict from the DataStructures.jl package, order should be preserved.
Cheers!

bkamins · 2020-11-16T17:59:51Z

Yes only Dict is sorted.

broke our code, probably major refactoring needed JuliaData/DataFrames.jl#2464

bkamins mentioned this pull request Oct 1, 2020

Deprecate DataFrame(::AbstractMatrix) #2433

Closed

bkamins added the breaking The proposed change is breaking. label Oct 1, 2020

bkamins added this to the 1.0 milestone Oct 1, 2020

oxinabox reviewed Oct 1, 2020

View reviewed changes

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved

oxinabox approved these changes Oct 1, 2020

View reviewed changes

nalimilan reviewed Oct 1, 2020

View reviewed changes

src/deprecated.jl Outdated Show resolved Hide resolved

src/deprecated.jl Show resolved Hide resolved

src/deprecated.jl Outdated Show resolved Hide resolved

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved

bkamins and others added 4 commits October 1, 2020 22:19

deprecate DataFrame constructors

5123d82

Update src/deprecated.jl

183418e

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

remove type specifications to simplify deprecations

c5629d4

update docstring

acff8d0

bkamins force-pushed the deprecate_constructors branch from 3a45426 to acff8d0 Compare October 1, 2020 20:31

bkamins mentioned this pull request Oct 2, 2020

Implement permutedims #2447

Merged

3 tasks

bkamins added 3 commits October 8, 2020 07:56

fix tests; part 1

ee464f8

fix tests part 2

cda327f

fix tests part 3

fa36598

bkamins added 2 commits October 9, 2020 09:39

Merge branch 'master' into deprecate_constructors

0af35eb

fix buggy corner cases in constructors

67e9b0a

bkamins mentioned this pull request Oct 9, 2020

Inconsistent handling of Tuple and NamedTuple JuliaData/Tables.jl#204

Closed

nalimilan reviewed Oct 13, 2020

View reviewed changes

bkamins and others added 3 commits October 13, 2020 12:41

Apply suggestions from code review

14d3b78

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

add comments from the review

c707173

Merge remote-tracking branch 'origin/deprecate_constructors' into dep…

b52dadb

…recate_constructors

oxinabox mentioned this pull request Oct 13, 2020

Stop supporting broadcasting + against whole DataFrames #2483

Closed

nalimilan reviewed Nov 2, 2020

View reviewed changes

bkamins and others added 3 commits November 3, 2020 09:47

Apply suggestions from code review

33fa8da

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

changes after code review

63e036e

Merge branch 'master' into deprecate_constructors

d5d012f

bkamins mentioned this pull request Nov 5, 2020

[BREAKING] Add PrettyTables.jl backend for printing DataFrames #2429

Merged

nalimilan reviewed Nov 5, 2020

View reviewed changes

make Dict constructor stricter

54f6fdf

nalimilan approved these changes Nov 6, 2020

View reviewed changes

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved

bkamins commented Nov 6, 2020

View reviewed changes

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved

Apply suggestions from code review

4b7b9fe

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

nalimilan reviewed Nov 6, 2020

View reviewed changes

src/dataframe/dataframe.jl Outdated Show resolved Hide resolved

Update src/dataframe/dataframe.jl

af68daf

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins merged commit 55533d1 into JuliaData:master Nov 6, 2020

bkamins deleted the deprecate_constructors branch November 6, 2020 21:55

JuliaRegistrator mentioned this pull request Nov 15, 2020

New version: DataFrames v0.22.0 JuliaRegistries/General#24650

Merged

khosravipasha added a commit to Tractables/LogicCircuits.jl that referenced this pull request Nov 22, 2020

rollback dataframes

6ad021e

broke our code, probably major refactoring needed JuliaData/DataFrames.jl#2464

khosravipasha added a commit to Tractables/ProbabilisticCircuits.jl that referenced this pull request Nov 22, 2020

rollback dataframes

3eb92cd

broke our code, probably major refactoring needed JuliaData/DataFrames.jl#2464

khosravipasha mentioned this pull request Nov 22, 2020

Move to DataFrames v0.22 Tractables/LogicCircuits.jl#57

Closed

pdeffebach mentioned this pull request Apr 17, 2021

ClipData alecloudenback/ExcelUtilities.jl#3

Closed

tmigot mentioned this pull request Jun 10, 2021

Bump DataFrames to 1 JuliaSmoothOptimizers/SolverBenchmark.jl#91

Merged

[BREAKING] deprecate DataFrame constructors #2464

[BREAKING] deprecate DataFrame constructors #2464

Conversation

bkamins commented Oct 1, 2020

matthieugomez commented Oct 3, 2020 via email

bkamins commented Oct 6, 2020

bkamins commented Oct 8, 2020

bkamins commented Oct 8, 2020

rofinn commented Oct 8, 2020 • edited Loading

bkamins commented Oct 9, 2020 • edited Loading

bkamins commented Oct 11, 2020

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

oxinabox Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Oct 13, 2020 • edited Loading

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

bkamins commented Nov 3, 2020

bkamins commented Nov 3, 2020

Choose a reason for hiding this comment

bkamins Nov 5, 2020 • edited Loading

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 Nov 6, 2020

bkamins commented Nov 6, 2020

bkamins commented Nov 6, 2020

0kto commented Nov 16, 2020 • edited Loading

pdeffebach commented Nov 16, 2020

bkamins commented Nov 16, 2020

0kto commented Nov 16, 2020

bkamins commented Nov 16, 2020

rofinn commented Oct 8, 2020 •

edited

Loading

bkamins commented Oct 9, 2020 •

edited

Loading

oxinabox Oct 13, 2020 •

edited

Loading

nalimilan Oct 13, 2020 •

edited

Loading

bkamins Nov 5, 2020 •

edited

Loading

0kto commented Nov 16, 2020 •

edited

Loading