Fix depwarns of getindex #1585

bkamins · 2018-11-01T23:15:52Z

I am in the process of cleaning up after getindex revision. I will continue as this is not finished (but it gives
the idea of changes).

A thing to discuss from what I have already changed is for hcat! - if we want a target functionality to put views or copies if RHS of hcat! is a SubDataFrame.

bkamins · 2018-11-02T08:03:31Z

@nalimilan Before I move forward I would like to discuss one design issue. In the transition period I need to write specialized code for SubDataFrame before it is defined in the package (e.g. in src/abstractdataframe/abstractdataframe.jl`). What do you think would be the best approach to handle this?

nalimilan · 2018-11-02T10:08:22Z

I'd rather define duplicated methods in deprecated.jl, so that the main code remains clean and they are easy to remove. That should also ensure SubDataFrame has already been defined.

bkamins · 2018-11-02T11:55:36Z

Good point - thanks.
So - for now I am left with the question about hcat!. The status is that currently hcat! for DataFrame passes a column without copying, but it does copy SubDataFrame columns.

The question is if we want to keep it
or
we should pass a view of a vector constituting the column of a SubDataFrame (so we would be consistent, in that hcat! produces an object that allows for modification of source data frames, but the downside is that we would have columns that are views not normal vector).

@nalimilan - do you have any opinion on this?

nalimilan · 2018-11-02T12:39:11Z

Tricky question! If we make copies automatically then there will be no (simple) way of concatenating a SubDataFrame while preserving views; conversely, if we don't make copies automatically, people can easily and efficiently call copy manually. As you said, it would also be consistent in that modifying the result would modify the inputs. So these are two arguments in favor of preserving views.

OTOH I'm not sure it's a very common need, and it makes things a bit more complex (creating SubArray columns). In terms of performance, preserving views is faster for one-off operations, but slower for repeated operations. In particular, if people use hcat! with by, we'd better preserve views or performance will be terrible.

Overall I'd lean towards preserving views, but it's a tough call.

bkamins · 2018-11-02T21:58:49Z

I am slowly moving forward. I make hcat! as you suggested.
Now the other point for you to look at is groupby where we make:

    sdf = df[cols]

selection. The tricky part is if we want to have sdf to be a SubDataFrame if df is a SubDataFrame (this is what would happen if I did not add a check as currently in this PR). This is related to groupby performance on SubDataFrame (maybe it is better that sdf is a DataFrame always - this is what is currently implemented in this PR).

nalimilan · 2018-11-03T14:45:22Z

I'm not sure it really matters whether we make a copy or not in groupby. Since we go over the grouping columns twice, copying could actually be a bit faster. Anyway I expect the difference to be negligible compared with the cost of grouping and combining (note that this is orthogonal to the fact that we pass a SubDataFrame to the user-provided function). So I'd go with the simplest solution.

nalimilan · 2018-11-03T14:38:42Z

test/dataframe.jl

+        @test all(ismissing, df[2])
+        @test all(ismissing, df[3])
+        # TODO: enable those tests after getindex deprecation period
+        # @test typeof(df[:, 1]) == Vector{Union{Int, Missing}}


Can you use @test_broken for these?

This is not broken - the test passes, but prints warnings (massively) and I understood that you wanted to avoid printing unnecessary warnings.

I think that's OK if the tests print warnings, as long as they are correct in the long term. Better have good tests that are temporarily verbose than having to enable them manually later.

So I understand we go the following route:

it is not a problem if tests print warnings;

however, we want to avoid printing warnings by library functions in normal usage.

Yes?

nalimilan · 2018-11-03T16:00:05Z

The amount of code that needs duplicating is terrifying. I wonder whether it wouldn't be simpler even for users to change the behavior without a deprecation. Indeed in most situations the deprecation will force people to write sdf[:, col] even if they don't actually need a copy. It's probably relatively rare to rely on the copying behavior. Though of course it could break code without notice. What do you think?

bkamins · 2018-11-03T17:08:40Z

I propose that I finish this PR (I will remove WIP then).
Then we can have a look how much copying was performed.
Actually - given your proposal - the code that is not needed is not that hard to track 😄.

However, I would retain deprecation - even for a short period to simplify debugging (even something what Julia 0.7 and 1.0 had - actually it worked well from my experience).

The big question is if we wait for setindex! changes and improved performance of groupby before the release (I would wait for it, but I am not sure what is the status of #1571).

bkamins · 2018-11-03T22:25:17Z

@nalimilan This should be good for a reivew now.
I have moved all offending code to src/deprecated.jl.
Also I have commented out some old tests and some new tests that throw a warning (I can uncomment them, but it it easier to read console log with them commented if you would want to).
The only tests that are massively offending that I retained turned on are for view construction. The upcoming change of meaning view(df, x) produces a lot of warnings but actually this is the change that is most likely to lead to subtle errors in the future.

nalimilan · 2018-11-04T12:07:36Z

src/abstractdataframe/abstractdataframe.jl

-        data = [df[name] for df in dfs]
+        # TODO: replace with commented out code after getindex deprecation
+        # data = [df[name] for df in dfs]
+        data = [df isa DataFrame ? df[name] : df[:, name] for df in dfs]


Wouldn't it be better to always use view here (even if that's temporary)? Same for completecases and maybe other places.

We could do this, but this would be breaking.
That is: currently df[name] when df is a SubDataFrame produces a copy and so I used df[:, name] to still make a copy temporarily (it will be a view after getindex deprecation).

Do you think making a view now would be better?

But here it's an implementation detail which shouldn't have any user-visible consequences, right?

Ah - right. I mixed it up with hcat! (this PR is really long). I will push a PR changing copy to view in places where it is not user visible.

I have pushed a PR implementing what you have requested. Actually it is a good way to test if something will not break in the future. The only limitation we have is with vcat with which you have started (I have noted it in the code).

nalimilan · 2018-11-04T12:16:48Z

OK, thanks, let's do this then, but remove the deprecations more quickly than we usually do.

bkamins · 2018-11-06T10:45:41Z

@nalimilan This should be cleaned up now for the next round of checking.

The only method that is left problematic is Base.getindex(itr::Cols, inds...) = getindex(itr.df, inds...) as it is sometimes called internally and occasionally can throw a warning, but this should not be much.

nalimilan · 2018-11-06T20:53:14Z

src/abstractdataframe/abstractdataframe.jl

+        # the code below assumes that only DataFrame and SubDataFrame
+        # are subtypes of AbstractDataFrame
+        # it should be removed ASAP after deprecation
+        data = [df isa DataFrame ? df[name] : view(parent(df)[name], rows(df)) for df in dfs]


It shouldn't matter much since it's temporary, but better call view only for SubDataFrame, so that the fallback works for any type. Then you don't need the scary comment.

But I cannot do this here because SubDataFrame is not defined at this stage of DataFrames.jl loading. And I would have to move the whole function much later in the loading process to make it work, which I think should not be done since it is only a temporary change.

Argh. Well, let's say we will remove this soon anyway...

bkamins · 2018-11-08T10:12:46Z

Good to merge (I do not want to do it myself, as it is too delicate 😄)?

nalimilan · 2018-11-08T18:30:50Z

So scary...

fixing depwarns part 1

0dde1ac

bkamins added 2 commits November 2, 2018 22:10

move SubDataFrame to deprecated.jl where possible

3ca914b

part two of cleanup

5575f90

nalimilan reviewed Nov 3, 2018

View reviewed changes

round 3 of fixes

3b69743

bkamins added 5 commits November 3, 2018 18:33

fixes round 4

20f8406

final cleanups

57bacf6

remove ambiguity of colwise

9c500f4

fix typo

fb1cc02

final fix

d8f9df4

bkamins changed the title ~~WIP: fix depwarns of getindex~~ Fix depwarns of getindex Nov 3, 2018

nalimilan reviewed Nov 4, 2018

View reviewed changes

bkamins added 5 commits November 5, 2018 12:15

create a view in SubDataFrame where possible

fd28944

fix typo

98df2a4

fix another typo

e46ac7d

more fixes

a330edc

final patch

2d91567

nalimilan reviewed Nov 6, 2018

View reviewed changes

nalimilan approved these changes Nov 6, 2018

View reviewed changes

bkamins mentioned this pull request Nov 8, 2018

Do we need columns and eachcol #1586

Closed

nalimilan merged commit d102854 into JuliaData:master Nov 8, 2018

bkamins deleted the fix_getindex_depwarns branch November 8, 2018 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix depwarns of getindex #1585

Fix depwarns of getindex #1585

bkamins commented Nov 1, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 2, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 2, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 3, 2018

nalimilan Nov 3, 2018

bkamins Nov 3, 2018

nalimilan Nov 3, 2018

bkamins Nov 3, 2018

nalimilan Nov 3, 2018

nalimilan commented Nov 3, 2018

bkamins commented Nov 3, 2018

bkamins commented Nov 3, 2018

nalimilan Nov 4, 2018

bkamins Nov 5, 2018

nalimilan Nov 5, 2018

bkamins Nov 5, 2018

bkamins Nov 5, 2018

nalimilan commented Nov 4, 2018

bkamins commented Nov 6, 2018

nalimilan Nov 6, 2018

bkamins Nov 6, 2018

nalimilan Nov 6, 2018

bkamins commented Nov 8, 2018

nalimilan commented Nov 8, 2018

Fix depwarns of getindex #1585

Fix depwarns of getindex #1585

Conversation

bkamins commented Nov 1, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 2, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 2, 2018

bkamins commented Nov 2, 2018

nalimilan commented Nov 3, 2018

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 commented Nov 3, 2018

bkamins commented Nov 3, 2018

bkamins commented Nov 3, 2018

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 commented Nov 4, 2018

bkamins commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkamins commented Nov 8, 2018

nalimilan commented Nov 8, 2018