-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BREAKING] fix isagg to correctly use a fast path #2357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I've been quite overoptimistic when I added this fast path... :-D
@@ -761,16 +765,37 @@ end | |||
Reduce(f, condf=nothing, adjust=nothing) = Reduce(f, condf, adjust, false) | |||
|
|||
check_aggregate(f::Any) = f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you combine validate_aggregate
with check_aggregate
? Basically, we have a fallback which returns f
, and for some combinations of function and type we return an optimized object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. And I did it now, but it was much easier to handle them separately when developing changes :)
check_aggregate(::typeof(maximum)) = Reduce(max) | ||
validate_aggregate(::typeof(maximum), ::AbstractVector{<:Union{Missing, Real}}) = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these methods work for any type. We just have a faster path when isconcretetype(S) && hasmethod($initf, Tuple{S})
, but we don't require typemin
and typemax
in general. In particular, we have code for CategoricalArray
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these methods work for any type.
I think we need this restriction:
julia> df = DataFrame(g=1, x=[[1,2,3], [1,2,3]])
2×2 DataFrame
│ Row │ g │ x │
│ │ Int64 │ Array… │
├─────┼───────┼───────────┤
│ 1 │ 1 │ [1, 2, 3] │
│ 2 │ 1 │ [1, 2, 3] │
julia> gdf = groupby(df, :g)
GroupedDataFrame with 1 group based on key: g
First Group (2 rows): g = 1
│ Row │ g │ x │
│ │ Int64 │ Array… │
├─────┼───────┼───────────┤
│ 1 │ 1 │ [1, 2, 3] │
│ 2 │ 1 │ [1, 2, 3] │
julia> combine(gdf, :x => maximum)
1×2 DataFrame
│ Row │ g │ x_maximum │
│ │ Int64 │ Array… │
├─────┼───────┼───────────┤
│ 1 │ 1 │ [1, 2, 3] │
julia> combine(gdf, :x => x -> maximum(x))
3×2 DataFrame
│ Row │ g │ x_function │
│ │ Int64 │ Int64 │
├─────┼───────┼────────────┤
│ 1 │ 1 │ 1 │
│ 2 │ 1 │ 2 │
│ 3 │ 1 │ 3 │
but I will make the signature looser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, again that multi-column issue... So yeah, the fast path needs to be disabled when the column can contain MULTI_COLS_TYPE
or AbstractVector
.
check_aggregate(::typeof(first)) = Aggregate(first) | ||
validate_aggregate(::typeof(first), v::AbstractVector) = eltype(v) === Any ? false : true | ||
validate_aggregate(::typeof(first), ::AbstractVector{<:Union{Missing, MULTI_COLS_TYPE, AbstractVector}}) = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this case is a bit ugly, as code that generates a single column when a data frame stores only scalars will suddenly create multiple columns when it stores e.g. named tuples. Of course this is a more general problem than first
and optimized reductions. I wonder whether we should require explicitly mentioning that you want the result to be destructured into multiple columns, e.g. via :x => MultiCol(x -> ...)
. Without this, we basically consider that only scalars should be stored in data frames, or many things may break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We indeed can discuss this and this relates to what @pdeffebach wants that we allow destruction into multiple columns.
Just to be clear the MULTI_COLS_TYPE
part is not really problematic in my opinion - it just makes sure we throw an error (as we disallow it now), so in the future we can process it correctly. This is what we try to do consistently everywhere, so that post 1.0 changes here will be non-breaking, as they will currently throw an error (this is your pet trick to handle SemVer AFAICT 😄).
A more problematic case is AbstractVector
as it was simply inconsistent between slow and fast paths (because fast path was not expanding it and slow path does pseudo-broadcasting).
So there actually two questions:
- if we want some
MultiCol
wrapper in the future or just allow multiple cols to be returned and silently accept it (I thought @pdeffebach wanted to silently accept this) - do we require to opt-in for pseudo-broadcasting, we never required it in the past, and it was the default, I think we should keep it.
Although it is a legacy from the very distant past. If I were designing it now I would never expand anything neither in rows nor in columns, just store one result per group in combine
and then say to users to use flatten
to flatten it. But this is a completely different design in comparison to what we have now, so this is just a side comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I had forgotten that we don't allow returning MULTI_COLS_TYPE
currently. So at least that's safe.
AbstractVector
remains problematic though. I wonder whether there are strong use cases for pseudo-broadcasting. It would be safer to make this opt-in for 1.0, otherwise we don't allow working with data frames whose cells contain vectors.
(BTW, instead for MultiCol
, maybe we could reuse AsTable
, but to wrap the returned value this time.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have thought about it when cleaning up the rules of pseudo-broadcasting recently. Here is what we have on 0.21:
julia> df = DataFrame(g=[1,2,3], x=[1:3, 4:6, 7:9])
3×2 DataFrame
│ Row │ g │ x │
│ │ Int64 │ UnitRan… │
├─────┼───────┼──────────┤
│ 1 │ 1 │ 1:3 │
│ 2 │ 2 │ 4:6 │
│ 3 │ 3 │ 7:9 │
julia> gdf = groupby(df, :g)
GroupedDataFrame with 3 groups based on key: g
First Group (1 row): g = 1
│ Row │ g │ x │
│ │ Int64 │ UnitRan… │
├─────┼───────┼──────────┤
│ 1 │ 1 │ 1:3 │
⋮
Last Group (1 row): g = 3
│ Row │ g │ x │
│ │ Int64 │ UnitRan… │
├─────┼───────┼──────────┤
│ 1 │ 3 │ 7:9 │
julia> combine(gdf, :x => first) # this is a wrong result and will be fixed by this PR to match what we have below
3×2 DataFrame
│ Row │ g │ x_first │
│ │ Int64 │ UnitRan… │
├─────┼───────┼──────────┤
│ 1 │ 1 │ 1:3 │
│ 2 │ 2 │ 4:6 │
│ 3 │ 3 │ 7:9 │
julia> combine(gdf, :x => x -> first(x)) # this is the intended output
9×2 DataFrame
│ Row │ g │ x_function │
│ │ Int64 │ Int64 │
├─────┼───────┼────────────┤
│ 1 │ 1 │ 1 │
│ 2 │ 1 │ 2 │
│ 3 │ 1 │ 3 │
│ 4 │ 2 │ 4 │
│ 5 │ 2 │ 5 │
│ 6 │ 2 │ 6 │
│ 7 │ 3 │ 7 │
│ 8 │ 3 │ 8 │
│ 9 │ 3 │ 9 │
julia> combine(gdf, :x => Ref∘first) # this is a currently intended method to protect from unwrapping - just like in standard broadcasting
3×2 DataFrame
│ Row │ g │ x_function │
│ │ Int64 │ UnitRange… │
├─────┼───────┼────────────┤
│ 1 │ 1 │ 1:3 │
│ 2 │ 2 │ 4:6 │
│ 3 │ 3 │ 7:9 │
So in short - we allow working with data frames that contain vectors as:
- normally vectors of vectors will be returned and it is not a problem
- if user unwraps the vector (like above - with
first
) thenRef
can be used as in Base to protect the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's continue this discussion in a separate issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK - can you please open the issue explaining what you would want to change? (given the three key considerations: 1) currently we unwrap vectors, 2) Ref
protects, 3) in the future we want to add support for multiple columns passing)
@@ -864,7 +911,11 @@ function groupreduce_init(op, condf, adjust, | |||
if isconcretetype(Tnm) && applicable(initf, Tnm) | |||
tmpv = initf(Tnm) | |||
initv = op(tmpv, tmpv) | |||
x = adjust isa Nothing ? initv : adjust(initv, 1) | |||
if adjust isa Nothing | |||
x = Tnm <: AbstractIrrational ? float(initv) : initv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too bad that irrationals don't define zero
and one
so that zero(x) + zero(x)
has the same type as x + x
... I don't remember offhand why they return Bool
but that may have to do with the fact that irrationals promote to whatever type they are combined with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not designed it 😄, but actually it is inconsistent:
julia> zero(pi) + pi
3.141592653589793
and you get Float64
. I have commented in JuliaLang/julia#36978 to keep track of it.
test/grouping.jl
Outdated
Union{Missing,Number}[1, 1.5, missing], Any[1, 1.5, missing]) | ||
gdf = groupby_checked(DataFrame(g=[1, 1, 1], x=col), :g) | ||
if fun isa typeof(last∘skipmissing) | ||
# this is another hard corner case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this special case? last(skipmissing(x))
doesn't work outside DataFrames (maybe it should though), and it doesn't seem to work with any type in DataFrames either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a special case, because it works in fast path, but fails in slow path. I have changed the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, it works only with GroupedDataFrame
input, not with a DataFrame
input. I don't get why.
Also, I thought the goal of this PR was to make the slow and fast path behave the same, so shouldn't this always throw an error whatever the eltype?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see the benefit of last∘skipmissing
throwing an error in cases when we can efficiently produce a correct result. The fact that last∘skipmissing
errors in Base is only due to O(1)
restriction in last
docstring, e.g. first∘skipmissing
works in base because its docstring does not require O(1)
. I think we should change in Base that last∘skipmissing
works.
it works only with
GroupedDataFrame
input, not with aDataFrame
input. I don't get why.
It should work in combine
both for GroupedDataFrame
and DataFrame
and with select
on GroupedDataFrame
(and pseudo-broadcasting is applied in this case). It is expected to fail for select
on DataFrame
. Do you observe a different behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, last(skipmissing(x))
and lastindex(skipmissing(x))
should probably be defined when x
is an AbstractArray
, since they don't require going over the whole collection.
It should work in
combine
both forGroupedDataFrame
andDataFrame
and withselect
onGroupedDataFrame
(and pseudo-broadcasting is applied in this case). It is expected to fail forselect
onDataFrame
. Do you observe a different behaviour?
I get this
julia> df = DataFrame(x=[1])
1×1 DataFrame
│ Row │ x │
│ │ Int64 │
├─────┼───────┤
│ 1 │ 1 │
julia> combine(df, :x => last ∘ skipmissing)
ERROR: MethodError: no method matching lastindex(::Base.SkipMissing{Array{Int64,1}})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I have forgotten of this path. So you have:
julia> df = DataFrame(x=[1])
1×1 DataFrame
│ Row │ x │
│ │ Int64 │
├─────┼───────┤
│ 1 │ 1 │
julia> combine(df, :x => last ∘ skipmissing)
ERROR: MethodError: no method matching lastindex(::Base.SkipMissing{Array{Int64,1}})
julia> combine(:x => last ∘ skipmissing, df)
1×1 DataFrame
│ Row │ x_last_skipmissing │
│ │ Int64 │
├─────┼────────────────────┤
│ 1 │ 1 │
which is unfortunate.
I would not touch it though now as it is not easy to fix.
In the long term we should rewrite the whole engine for select
/transform
/combine
to be unified. Now - unfortunately - I have designed it in stages, and when initially implementing select
and transform
we have not envisioned that we will have a full ecosystem that we have now, so we essentially have two processing engines - one for combine
in GroupedDataFrame
and the other for select
in DataFrame
.
This should be implemented in a way that select
/transform
/combine
on DataFrame
are always essentially calls to GroupedDataFrame
on a group formed by no columns (single group). Additional things to remember when implementing it:
- add better support for 0 groups
- add possibility for a group to have zero rows
- add requested extensions to the syntax of
source_columns => function => target_col_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. No hurry, maybe just copy your comment to an issue to keep your analysis available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this is tracked in separate issues.
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
And adding checks you required caught another inconsistency - this time in |
|
check_aggregate(::typeof(first)) = Aggregate(first) | ||
validate_aggregate(::typeof(first), v::AbstractVector) = eltype(v) === Any ? false : true | ||
validate_aggregate(::typeof(first), ::AbstractVector{<:Union{Missing, MULTI_COLS_TYPE, AbstractVector}}) = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I had forgotten that we don't allow returning MULTI_COLS_TYPE
currently. So at least that's safe.
AbstractVector
remains problematic though. I wonder whether there are strong use cases for pseudo-broadcasting. It would be safer to make this opt-in for 1.0, otherwise we don't allow working with data frames whose cells contain vectors.
(BTW, instead for MultiCol
, maybe we could reuse AsTable
, but to wrap the returned value this time.)
Maybe it would be simpler to disable the fast path for |
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
I can do it if you prefer. It will be a bit simpler code, but actually more lines of code (as I have to add special methods for this case, currently this is just 2 additional conditions) |
Whatever you think is the cleanest. |
Thank you! |
Fixes #2316
also is related to https://github.com/JuliaLang/Statistics.jl/issues/50 and JuliaLang/julia#36978 but I work around it.
@pdeffebach - this PR uncovered many corner cases, a close look at what I propose to do would be welcome.