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.

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

fix collect on stateful iterators #42172

Merged
merged 4 commits into from
Sep 14, 2021
Merged

fix collect on stateful iterators #42172

merged 4 commits into from
Sep 14, 2021

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Sep 9, 2021

Generalization of #41919
Fixes #42168

Needed to also fix some ssair code, which was unnecessarily relying on inference recursively, and led to some other fixes.

@vtjnash vtjnash added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 9, 2021
@KristofferC
Copy link
Sponsor Member

This requires a manual backport against backports-release-1.6.

@bramtayl
Copy link
Contributor

This seems to have broken Unzip.jl. Is the type assert really necessary?

julia> using Unzip

julia> function unstable(x)
           if x == 2
               (x, x + 0.0, x, x + 0.0)
           else
               (x, x + 0.0)
           end
       end
unstable (generic function with 1 method)

julia> unzip(Iterators.map(unstable, 1:3))
ERROR: TypeError: in typeassert, expected AbstractVector{<:Tuple{Int64, Float64, Vararg{Real}}}, got a value of type Unzip.Rows{Tuple{Int64, Float64, Union{Missing, Int64}, Union{Missing, Float64}}, 1, Tuple{Vector{Int64}, Vector{Float64}, Vector{Union{Missing, Int64}}, Vector{Union{Missing, Float64}}}}
Stacktrace:
 [1] _collect(c::Unzip.Rows{Tuple{}, 1, Tuple{}}, itr::Base.Generator{UnitRange{Int64}, typeof(unstable)}, #unused#::Base.EltypeUnknown, isz::Base.HasShape{1})
   @ Base .\array.jl:754
 [2] unzip(rows::Base.Generator{UnitRange{Int64}, typeof(unstable)})
   @ Unzip C:\Users\brand\.julia\dev\Unzip\src\Unzip.jl:282
 [3] top-level scope
   @ REPL[14]:1

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Sep 1, 2022

It looks like Unzip has a bug where it accidentally tries to return a Missing value in the result, where no such value existed initially?

@bramtayl
Copy link
Contributor

bramtayl commented Sep 2, 2022

Nope, that's not a bug, it's a choice. Unzip will fill in missing for iterators of differently sized tuples:

unzip([(1, 'a'), (2,), (3, 'b')] = ([1, 2, 3], [1, missing, 'b'])

I found this while rewriting, so this is no longer an issue, but.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Sep 2, 2022

zip is expected to truncate to the shortest input

@bramtayl
Copy link
Contributor

bramtayl commented Sep 2, 2022

Hmm, well, I guess that implies that zip could never provide tuples of different sizes, so unzipping tuples of different sizes is undefined...I could error here, which would make the code a lot simpler but also less fun. As of now, I have my own version of the collect method, without a type assert, that will error for empty iterators if I can't guess the column types. https://github.com/bramtayl/Unzip.jl/blob/c239114b4b7faf71dab924c1832a11eaa0312d22/src/Unzip.jl#L317

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problematic interaction between Stateful and map
4 participants