Add :setequal to cols kwarg in vcat, push! and append! #1958

bkamins · 2019-09-13T22:38:14Z

Fixes #1904

I think we should make vcat and push! consistent with the name of kwarg. Should it be cols (now used in vcat) or columns (now used in push!). Any opinions?

nalimilan · 2019-09-18T16:53:36Z

Good catch. I guess we should use cols everywhere.

bkamins · 2019-09-27T07:40:57Z

This PR also should be good for a review

bkamins · 2019-10-19T19:31:03Z

This adds :subset and :union to push!. When this is decided that we like it and would be merged I will open a PR for append! to add the same functionality there.

@rapus95: could you please have a look:

if the functionality is what you wanted
is the test coverage OK
and of course - if you can spot any bugs in the implementation

Thank you!

CarloLucibello · 2019-10-20T15:19:47Z

this PR should also cover the following case, right?

df = DataFrame()
push!(df, (a=1, b=2, c=3), cols=:union)

I would found this to be very convenient in practice when I store in a dataframe some observations taken during the iterations of a simulation, but while prototyping I change my mind quite often about which observations actually storing.

bkamins · 2019-10-20T17:16:11Z

Actually:

df = DataFrame()
push!(df, (a=1, b=2, c=3))

works already.

This PR would cover the following case additionally:

df = DataFrame(a=0,d=4)
push!(df, (a=1, b=2, c=3), cols=:union)

and df would contain columns [:a,:d,:b,:c]. But I guess this is what you want - right?

CarloLucibello · 2019-10-20T20:25:33Z

right, I don't know why I led myself to believe that didn't work. sorry for the noise

bkamins · 2019-10-20T20:35:13Z

No problem - it is better to be explicit - let us just make sure that what we provide is what users want and would use 😄.

rapus95

This took a lot of time... I hope some of the comments are helpful to you.

src/abstractdataframe/abstractdataframe.jl

src/dataframe/dataframe.jl

src/abstractdataframe/abstractdataframe.jl

src/dataframe/dataframe.jl

rapus95 · 2019-10-21T11:35:41Z

src/dataframerow/dataframerow.jl

+function Base.push!(df::DataFrame, dfr::DataFrameRow; cols::Symbol=:equal,
+                    columns::Union{Nothing,Symbol}=nothing)
+    if isnothing(columns)
+        columns = cols


Same as for the other push! (why not rename every columns)

src/dataframerow/dataframerow.jl

test/dataframe.jl

src/abstractdataframe/abstractdataframe.jl

src/dataframe/dataframe.jl

rapus95 · 2019-10-21T14:08:44Z

How about:

"""
[...] vcat(left, right, cols=X)
where for X you have the following options:
 * :identical, order and names of columns need to match
 * :equal, names of columns need to match (order is not necessary)
 * :intersect, newnames* = names(left) \cap names(right)
 * :subset, newnames* = names(left)
 * :union, newnames* = names(left) \cup names(right)
* missing data is filled with `missing`
"""

rapus95 · 2019-10-21T14:21:57Z

Btw what should happen in that case:

df1 = DataFrame([[1], [1]], [:A, :C])
df2 = DataFrame([[2], [2]], [:B, :D])
df3 = vcat(df1, df2, cols=:subset)
df4 = push!(df1, df2[1,:], cols=:subset)

That situation would make a good test case I guess as it helps spot changing behaviour.

bkamins · 2019-11-03T10:08:49Z

This is what I came up with after pondering on it:

I removed :union option in push! for now; it changed column list/types in the destination data frame, which requires more thought how to do it best (type promotion etc. - as discussed) and is a big change that should be done in a separate PR
I left all other options as they are not problematic I think
I left the deprecation of columns to cols (which should make subsequent PRs easier as they will not have to handle this problem)

bkamins · 2019-11-03T11:09:43Z

I have also updated the tests to take into account the changes we make here.

There are two notes for the future:

if we use reduce in vcat in the future we will require the columns to be :identical - (now we allow :equal) - I think it is OK
tests of push! etc. will require a major cleanup after we make further changes in the functionality (but this is also OK, now I have to handle both deprecation of :columns to :cols and :equal to :identical and in the next release these deprecation will be gone, so it will be possible to simplify testing code)

I have also resolved all comments (there were many of them so by resolving I kept track of what was fixed). Thank you for contributing to this PR.

bkamins · 2019-11-08T13:40:04Z

How I will update this PR soon (hopefully today :)):

:identical will get renamed to :equal
:equal will be renamed to :setequal
:setequal is to be a default for push!, vcat and append! (the last change is a separate PR, see Add :equal support in append! #2007)

src/dataframe/dataframe.jl

bkamins · 2019-11-29T16:40:14Z

This should be good to have a look at (especially test coverage as we added many options in this PR). Thank you!.

src/dataframe/dataframe.jl

nalimilan

Sorry, I've spotted more stylistic things. Since I have still one substantial point, probably worth fixing them too.

src/dataframe/dataframe.jl

src/dataframerow/dataframerow.jl

src/other/tables.jl

src/dataframe/dataframe.jl

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

bkamins · 2019-12-01T11:14:31Z

@nalimilan - as a second thought - given the discussion in #1993 we sort columns of Dict maybe we should allow appending of Dict and then also perform sorting of columns in push! and allow it there?

In other words - the constructor considers the order of columns in Dict as defined (sorted), so maybe we just should propagate this rule (and document it).

nalimilan · 2019-12-01T11:27:37Z

Well it sounds OK to sort column names by default, but if you pass cols=:orderequal then you really care about the order, so it would be weird to accept Dict. Also it's always better to be strict as this can be changed later without breaking...

bkamins · 2019-12-01T11:33:59Z

OK

bkamins · 2019-12-01T13:25:58Z

Thank you for working on it.

bkamins changed the title ~~WIP vcat push identical~~ Add :identical to vcat and push Sep 26, 2019

bkamins changed the title ~~Add :identical to vcat and push~~ Add :identical to cols kwarg in vcat and push Sep 26, 2019

bkamins force-pushed the vcat_push_identical branch from cc50ea0 to 6a1adaf Compare October 5, 2019 10:39

bkamins changed the title ~~Add :identical to cols kwarg in vcat and push~~ Add :identical to cols kwarg in vcat and push! Oct 11, 2019

bkamins mentioned this pull request Oct 11, 2019

vcat! or push!(..., columns=:union) #1982

Closed

bkamins added 6 commits October 19, 2019 20:12

add :identical to vcat and push!

38c209b

add deprecation warning

01804d9

deprecate columns

3c22bf7

add tests

38a65fa

fix bug in _vcat

2c7be06

small fixes

699859b

bkamins force-pushed the vcat_push_identical branch from 6a1adaf to 699859b Compare October 19, 2019 18:13

add :subset and :union to push!

755b96e

rapus95 reviewed Oct 21, 2019

View reviewed changes

nalimilan reviewed Oct 21, 2019

View reviewed changes

apply review changes and remove :union option in push!

65acd64

fix tests

645ce60

bkamins mentioned this pull request Nov 8, 2019

Add :equal support in append! #2007

Closed

bkamins added 6 commits November 25, 2019 18:53

Merge branch 'master' into vcat_push_identical

4cd0689

improve error messages

e5b9091

fix old tests

4c2be0f

fix more tests

b50b397

another fix

99f44c1

make cols a kwarg in append!

d032747

bkamins mentioned this pull request Nov 26, 2019

Switch cols to kwarg from positional args #2023

Closed

fix a bug in :setequal

301896b

rapus95 reviewed Nov 27, 2019

View reviewed changes

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

bkamins added 5 commits November 29, 2019 00:42

change :orderequal case for Dict

073bd83

remove comment

c1cfdda

fix docs and tests

c61b8a5

fix test

dc72c66

fix :orderequal for ordered dicts

60eb1b6

bkamins mentioned this pull request Nov 30, 2019

Release 0.20.0 #2028

Merged

4 tasks

nalimilan reviewed Nov 30, 2019

View reviewed changes

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

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

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

add fixes after the review

6bccdb6

nalimilan reviewed Nov 30, 2019

View reviewed changes

bkamins and others added 3 commits November 30, 2019 22:46

Apply suggestions from code review

475e221

Co-Authored-By: Milan Bouchet-Valat <nalimilan@club.fr>

fixes after code review

45abe23

Fix

c43d1a6

nalimilan approved these changes Dec 1, 2019

View reviewed changes

bkamins merged commit c1d0ae4 into JuliaData:master Dec 1, 2019

bkamins deleted the vcat_push_identical branch December 1, 2019 13:25

bkamins mentioned this pull request Dec 1, 2019

Merge meanings of cols keyword arguments between push! and vcat #1991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add :setequal to cols kwarg in vcat, push! and append! #1958

Add :setequal to cols kwarg in vcat, push! and append! #1958

bkamins commented Sep 13, 2019 •

edited

Loading

nalimilan commented Sep 18, 2019

bkamins commented Sep 27, 2019

bkamins commented Oct 19, 2019

CarloLucibello commented Oct 20, 2019 •

edited

Loading

bkamins commented Oct 20, 2019

CarloLucibello commented Oct 20, 2019

bkamins commented Oct 20, 2019

rapus95 left a comment

rapus95 Oct 21, 2019

rapus95 commented Oct 21, 2019

rapus95 commented Oct 21, 2019

bkamins commented Nov 3, 2019

bkamins commented Nov 3, 2019

bkamins commented Nov 8, 2019

bkamins commented Nov 29, 2019

nalimilan left a comment

bkamins commented Dec 1, 2019

nalimilan commented Dec 1, 2019

bkamins commented Dec 1, 2019

bkamins commented Dec 1, 2019

Add :setequal to cols kwarg in vcat, push! and append! #1958

Add :setequal to cols kwarg in vcat, push! and append! #1958

Conversation

bkamins commented Sep 13, 2019 • edited Loading

nalimilan commented Sep 18, 2019

bkamins commented Sep 27, 2019

bkamins commented Oct 19, 2019

CarloLucibello commented Oct 20, 2019 • edited Loading

bkamins commented Oct 20, 2019

CarloLucibello commented Oct 20, 2019

bkamins commented Oct 20, 2019

rapus95 left a comment

Choose a reason for hiding this comment

rapus95 Oct 21, 2019

Choose a reason for hiding this comment

rapus95 commented Oct 21, 2019

rapus95 commented Oct 21, 2019

bkamins commented Nov 3, 2019

bkamins commented Nov 3, 2019

bkamins commented Nov 8, 2019

bkamins commented Nov 29, 2019

nalimilan left a comment

Choose a reason for hiding this comment

bkamins commented Dec 1, 2019

nalimilan commented Dec 1, 2019

bkamins commented Dec 1, 2019

bkamins commented Dec 1, 2019

bkamins commented Sep 13, 2019 •

edited

Loading

CarloLucibello commented Oct 20, 2019 •

edited

Loading