[BREAKING] make id_vars go first in stack #2147
Conversation
bkamins
changed the title
src/abstractdataframe/reshape.jl
Outdated
| @@ -55,14 +57,19 @@ function stack(df::AbstractDataFrame, measure_vars::AbstractVector{<:Integer}, | |||
| id_vars::AbstractVector{<:Integer}; variable_name::Symbol=:variable, | |||
| value_name::Symbol=:value, view::Bool=false, | |||
| variable_eltype::Type=CategoricalString) | |||
| if stack_didnotwarn | |||
There was a problem hiding this comment.
I'm not sure it's a good idea to print this. There's no way to turn this off so it will annoying everybody. And if you relied on the columns order, you'll find out the problem soon enough.
The only way to deprecate this cleanly would be to introduce a keyword argument, but that would be quite annoying.
There was a problem hiding this comment.
The point is that I did not do a normal deprecation exactly because I do not want the message to be annoying. It will be printed only once per Julia session (as opposed a normal depwarn that would be printed every time).
The deprecation would only be in 0.21, in 1.0 release it will be gone.
@oxinabox, @pdeffebach - can you please comment on it from a "user's voice" perspective. Thank you!
There was a problem hiding this comment.
as opposed a normal depwarn that would be printed every time).
is that still not fixed?
Rather than using a global you can do @warn msg maxlog=1
to prevent warning showing up more than once
I never use stack, only unstack, so i have no stake in this game.
Warnings are, in general are incredibly annoying.
I would rather a well-documented and breaking change than most warnings.
There was a problem hiding this comment.
@warn msg maxlog=1
Thank you - one learns something all the time (I knew there should be a solution for this 😄). If we end up printing a deprecation warning I will change the implementation to use your tip.
|
I want to add that I think this is a good idea and I wonder if, despite allowing indexing by numbers, the order of columns shouldn't be considered a part of the API that we can "break". i.e., tell users not to rely on the order to columns in their code. |
This probably should be a soft recommendation (as it is in other tabular data systems where you are typically advised to index column by name). But in general sometimes indexing by a position is useful (and that is why we provide it) so I would be hesitant just to say that "the order of columns is undefined". Anyway - the decision for this PR is to either
or
(I would prefer option 1. as printing a warning only once is not that bad I think) |
|
I vote for 2. IIUC @oxinabox has the same preference, but he said he has no stake in this game, so... :-) |
|
Well it seems only I want this warning to be printed. I have dropped it in the commit I have just pushed. |
|
Thank you! |
Fixes #645.
Now we put
id_varsfirst in the data frame returned fromstack