Skip to content
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.

Sign up for GitHub

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

Improve foldl's stability on nested Iterators. #45789

Merged
merged 5 commits into from
Jun 28, 2022
Merged

Improve foldl's stability on nested Iterators. #45789

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3112487
Inference improvement.
N5N3 Jun 23, 2022
393ffb0
Add test.
N5N3 Jun 23, 2022
a929310
Avoid invalid `Core.Typeof`
N5N3 Jun 23, 2022
8e9d35f
Improve `(c::ComposedFunction)(x...)`'s inferability
N5N3 Jun 20, 2022
f7ab814
Merge branch 'master' into recursion_inference
aviatesk Jun 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions base/operators.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,8 +1078,7 @@ struct Fix1{F,T} <: Function
f::F
x::T

Fix1(f::F, x::T) where {F,T} = new{F,T}(f, x)
Fix1(f::Type{F}, x::T) where {F,T} = new{Type{F},T}(f, x)
Fix1(f, x) = new{Core.Typeof(f),Core.Typeof(x)}(f, x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This usage of Core.Typeof is not correct, because Typeof doesn't always return legal types:

julia> struct Fix1{F,T} <: Function
           f::F
           x::T

           Fix1(f, x) = new{Core.Typeof(f),Core.Typeof(x)}(f, x)
       end

julia> Fix1(==, Vector{Core._typevar(:T, Union{}, Any)})
ERROR: TypeError: in new, expected DataType, got Type{Fix1{typeof(==), Type{Array{T, 1}}}}
Stacktrace:
 [1] Fix1(f::Function, x::Type)
   @ Main ./REPL[3]:5
 [2] top-level scope
   @ REPL[4]:1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use it for f? (I see ComposedFunction(outer, inner) = new{Core.Typeof(outer),Core.Typeof(inner)}(outer, inner) above)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to define methods on incomplete types with free typevars, but it will change the kind of error thrown to a TypeError during construction instead of a MethodError when actually called, so it's not great either

Copy link
Member Author

@N5N3 N5N3 Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried f(::Type{T}) where {T} = T, but f(Vector{Core._typevar(:T, Union{}, Any)}) throws a ERROR: UndefVarError: T not defined
So we need something like:

_typeof(x) = typeof(x)
_typeof(::Type{T}) where {T} = @isdefined(T) ? Type{T} : DataType

?
If this is acceptable, should we also modifies

julia/base/operators.jl

Line 931 in 68d62ab

Returns(value) = new{Core.Typeof(value)}(value)

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything we should talk at this point, or is this PR ready for merge?

Copy link
Member Author

@N5N3 N5N3 Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with incomplete type. If core devs are happy with the change in a929310, I think it’s ready to merge.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged, thanks

end

(f::Fix1)(y) = f.f(f.x, y)
Expand All @@ -1095,8 +1094,7 @@ struct Fix2{F,T} <: Function
f::F
x::T

Fix2(f::F, x::T) where {F,T} = new{F,T}(f, x)
Fix2(f::Type{F}, x::T) where {F,T} = new{Type{F},T}(f, x)
Fix2(f, x) = new{Core.Typeof(f),Core.Typeof(x)}(f, x)
end

(f::Fix2)(y) = f.f(y, f.x)
Expand Down
33 changes: 22 additions & 11 deletions base/reduce.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,17 +140,28 @@ what is returned is `itr′` and

op′ = (xfₙ ∘ ... ∘ xf₂ ∘ xf₁)(op)
"""
_xfadjoint(op, itr) = (op, itr)
_xfadjoint(op, itr::Generator) =
if itr.f === identity
_xfadjoint(op, itr.iter)
else
_xfadjoint(MappingRF(itr.f, op), itr.iter)
end
_xfadjoint(op, itr::Filter) =
_xfadjoint(FilteringRF(itr.flt, op), itr.itr)
_xfadjoint(op, itr::Flatten) =
_xfadjoint(FlatteningRF(op), itr.it)
function _xfadjoint(op, itr)
itr′, wraps = _xfadjoint_unwrap(itr)
_xfadjoint_wrap(op, wraps...), itr′
end

_xfadjoint_unwrap(itr) = itr, ()
function _xfadjoint_unwrap(itr::Generator)
itr′, wraps = _xfadjoint_unwrap(itr.iter)
itr.f === identity && return itr′, wraps
return itr′, (Fix1(MappingRF, itr.f), wraps...)
end
function _xfadjoint_unwrap(itr::Filter)
itr′, wraps = _xfadjoint_unwrap(itr.itr)
return itr′, (Fix1(FilteringRF, itr.flt), wraps...)
end
function _xfadjoint_unwrap(itr::Flatten)
itr′, wraps = _xfadjoint_unwrap(itr.it)
return itr′, (FlatteningRF, wraps...)
end

_xfadjoint_wrap(op, f1, fs...) = _xfadjoint_wrap(f1(op), fs...)
_xfadjoint_wrap(op) = op

"""
mapfoldl(f, op, itr; [init])
Expand Down
13 changes: 13 additions & 0 deletions test/reduce.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,3 +677,16 @@ end
@test mapreduce(+, +, oa, oa) == 2len
end
end

# issue #45748
@testset "foldl's stability for nested Iterators" begin
a = Iterators.flatten((1:3, 1:3))
b = (2i for i in a if i > 0)
c = Base.Generator(Float64, b)
d = (sin(i) for i in c if i > 0)
@test @inferred(sum(d)) == sum(collect(d))
@test @inferred(extrema(d)) == extrema(collect(d))
@test @inferred(maximum(c)) == maximum(collect(c))
@test @inferred(prod(b)) == prod(collect(b))
@test @inferred(minimum(a)) == minimum(collect(a))
end