WIP/ RFC/First PR: Add `unzip` to base.iterators.jl #21208

miguelraz · 2017-03-28T21:46:05Z

~~@StefanKarpinski~~ Julia needs an underling to give this PR a shot. #13942.

I know I'm clearly missing some important things, but I hope the effort counts. I couldn't find a way to get the rows from the array comprehension...
Willing to start over from scratch - any comments/critiques/suggestions more than welcome.
I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed?

[ci skip]

Roadmap:

Verify each returned array is the same type as the types of the elements
Verify lengths of all arrays to be unzipped
Return as a single Array of Arrays or as separate Arrays?
Remove that zip is its own inverse in @doc zip?
Revise documentation phrasing and example

@StefanKarpinski

~~@StefanKarpinski~~ Julia needs an underling to give this PR a shot. [#13942](#13942). 1. I know I'm clearly missing some important things, but I hope the effort counts. I couldn't find a way to get the rows from the array comprehension... 2. Willing to start over from scratch - any comments/critiques/suggestions more than welcome. 3. I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed? [ci skip] Roadmap: * Verify each returned array is the same type as the types of the elements * Verify lengths of all arrays to be unzipped * Return as a single Array of Arrays or as separate Arrays? * Remove that `zip is its own inverse` in `@doc zip`? * Revise documentation phrasing and example

JeffBezanson · 2017-03-28T22:05:39Z

I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed?

Definitely; I almost always start with examples and then generalize.

If you want an array of arrays, you can use nested comprehensions.

Ideally this would work with arbitrary iterators, which do not necessarily support length and indexing. However it might be reasonable to assume that the objects inside the outer iterator do support indexing; this is certainly the case for the result of zip.

Remove that zip is its own inverse in @doc zip?

+1 to that

update julia fork

@StefanKarpinski

~~@StefanKarpinski~~ Julia needs an underling to give this PR a shot. [#13942](#13942). 1. I know I'm clearly missing some important things, but I hope the effort counts. I couldn't find a way to get the rows from the array comprehension... 2. Willing to start over from scratch - any comments/critiques/suggestions more than welcome. 3. I am first trying a MWE with simple structure and inputs. Then I will try to get something that works more generically. Is this a good way to procceed? [ci skip] Roadmap: * Verify each returned array is the same type as the types of the elements * Verify lengths of all arrays to be unzipped * Return as a single Array of Arrays or as separate Arrays? * Remove that `zip is its own inverse` in `@doc zip`? * Revise documentation phrasing and example

ararslan · 2017-03-28T22:50:26Z

If I may offer a couple suggestions, I would have the function return a tuple of iterators rather than an array of arrays, and I would restrict unzip to Zip objects. As an example:

unzip(iter::AbstractZipIterator) = _unzip(iter)
# This avoids exporting the method that isn't for AbstractZipIterators
_unzip(iter::Zip1) = (iter.a,)
_unzip(iter::Zip2) = (iter.a, iter.b)
_unzip(iter::Zip) = (iter.a, _unzip(iter.z)...)
_unzip(iter) = (iter,)

This makes use of the internal structure of the Zip objects to preserve the iterators that were passed to zip.

miguelraz · 2017-03-28T23:11:01Z

Welcome!

@JeffBezanson Thanks! Been lurking for a while. Time to be brave and attempt a commit.

If you want an array of arrays, you can use nested comprehensions.

Tried quite a few variations of that - still haven't got the hang of it yet.
I think I will go with @ararslan's advice and use the Zip object structre that is already in place - though I confess those functions still look like voodoo magic to me, so I am going to just copy paste this until I get a better hang of it.

I will try following the advice that it says to push onto the patch-3 branch on my own fork.
I have added tests and will need a comment on those too, if anyone is willing to help.
Will report results.

miguelraz · 2017-03-28T23:30:46Z

@kshyatt Added tests

ararslan · 2017-03-28T23:36:20Z

though I confess those functions still look like voodoo magic to me

I'll try to break it down so it's more understandable.

First we're defining a function to export called unzip that accepts objects of type Base.Iterators.AbstractZipIterator. The goal here is to losslessly (or as much so as we can) recover the input passed to zip.

unzip(iter::AbstractZipIterator) = _unzip(iter)

We're just making it call an internal, unexported function _unzip. Doing it this way ensures that when someone goes to use the unzip function, the only visible method is the one that takes AbstractZipIterators.

Now we define the internal function. This exploits the structure of the Zip objects. Here are the objects as defined in base/iterators.jl:

struct Zip1{I} <: AbstractZipIterator
    a::I
end

struct Zip2{I1, I2} <: AbstractZipIterator
    a::I1
    b::I2
end

struct Zip{I, Z<:AbstractZipIterator} <: AbstractZipIterator
    a::I
    z::Z
end

For Zip1, we see it has 1 field called a. So we add a method to _unzip to extract the a field, then wrap it in a tuple. This is

_unzip(iter::Zip1) = (iter.a,)

For Zip2, there are 2 fields, a and b, so we do the same as for Zip1:

_unzip(iter::Zip2) = (iter.a, iter.b)

Zip is a little different; one field is an iterator and the other is another Zip. Thus we can call _unzip recursively to completely unzip. To ensure we don't get nested tuples, we splat the result. This is

_unzip(iter::Zip) = (iter.a, _unzip(iter.z)...)

Then we define a fallback method that just wraps the input in a tuple. This is a safeguard against any potential infinite recursion in the Zip method.

_unzip(iter) = (iter,)

And that's it! Seem any less magical?

miguelraz · 2017-03-28T23:52:23Z

Damn, this is why I love Julia.
It definitely looks way less magical now!
@ararslan a trillion internet points to you kind sir.
I kinda started thinking that they were trying to setup a divide and conque algo, but it is now crystal clear.

I guess I have some questions still though:
Why do we like to return tuples so much? Why not arrays?

I have no idea how to proceed now, though I just added edited the doc lines to make sure that zip and unzip are inverses of each other as seconded by Jeff.

StefanKarpinski · 2017-03-29T00:00:51Z

Why do we like to return tuples so much? Why not arrays?

Type inference can reason about tuples and the types of individual slots in them. Inference can only reason about the element type of an entire array, which in this case is not nearly as helpful.

StefanKarpinski · 2017-03-29T00:01:06Z

Thanks for taking a crack at this @miguelraz!

miguelraz · 2017-03-29T00:09:18Z

@StefanKarpinski Thanks to you guys for helping me learn!
Any comments/criticism/suggestions/advice more than welcome.
I won't promise results, but I will try my best at it.

Type inference can reason about tuples and the types of individual slots in them

Aaaah. And inference leads to speed. No wonder Base likes Tuples so much.

fredrikekre · 2017-03-29T05:54:28Z

test/iterators.jl

+# unzip
+let z = zip(1:2)
+    @test unzip(z) == 1:2
+    @test eltype(unzip(z)) == UnitRange{Int64}


UnitRange{Int} here since it will fail on systems with Int == Int32 (and same on the tests below)

Thanks! Corrected now.

fredrikekre · 2017-03-29T05:59:52Z

base/iterators.jl

@@ -240,6 +240,32 @@ end
 iteratorsize{I1,I2}(::Type{Zip{I1,I2}}) = zip_iteratorsize(iteratorsize(I1),iteratorsize(I2))
 iteratoreltype{I1,I2}(::Type{Zip{I1,I2}}) = and_iteratoreltype(iteratoreltype(I1),iteratoreltype(I2))

+# unzip
+```jldoctest


Only the code should be within ```jldoctest ... ``` (i.e. the parts with julia>)

Also, the complete docstring should be within """ ... """, take a look at the docstring for zip on the lines above in this file!

(Also note that the examples are outdated after you updated unzip to only work with AbstractZipIterators)

OK, tried to fix this up.
Review comments?

fredrikekre · 2017-03-29T06:00:55Z

base/iterators.jl

+For iterable objects of the same length, returns iterable sets, where the ith set contains
+the ith component of each input iterable object.
+
+Note that the inverse of [`unzip`](@ref) is [`zip`](@ref).


Perhaps not necessary to reference unzip in its own documentation?

But zip's docs say that zip's inverse is unzip.
(Or at least tried to indicate the inverse.)
... Decision time?

I just meant the @ref part of unzip here, no need to link to itself. (And the docs for zip probably shouldn't have a @ref for itself either.)

martinholters · 2017-03-29T09:28:47Z

While I agree that an unzip specialized for zip output makes a lot of sense, I also think a more general unzip that can handle any iterator that delivers tuples of constant size would be quite useful. That has not to be part of this PR, I just wanted to mention it before someone closes #13942 after this has been merged.

StefanKarpinski · 2017-03-29T14:03:03Z

I would imagine that calling unzip on a zip would generally only be done in tests. After all, if you just zipped something, why are you unzipping it again?

ararslan · 2017-03-29T17:26:05Z

base/iterators.jl

+# This avoids exporting the method that isn't for AbstractZipIterators
+_unzip(iter::Zip1) = (iter.a,)
+_unzip(iter::Zip2) = (iter.a, iter.b)
+_unzip(iter::Zip) = (iter.a, unzip(iter.z)...)


The inner call to unzip here should actually be _unzip. (That was my typo initially, sorry about that.)

Gotcha. Corrected now.

ararslan · 2017-03-29T17:26:48Z

test/iterators.jl

@@ -22,6 +22,22 @@ let z = zip(1:2, 3:4, 5:6)
    @test eltype(z) == Tuple{Int,Int,Int}
 end

+# unzip
+let z = zip(1:2)
+    @test unzip(z) == 1:2


Since unzip always returns a Tuple, the right hand side of this test should be (1:2,)

Gotcha. Corrected as well.

ararslan · 2017-03-29T17:28:06Z

After all, if you just zipped something, why are you unzipping it again?

That's a fair point, but if you haven't zipped something, why would you be unzipping it at all?

dmbates · 2017-03-29T17:41:01Z

On Wed, Mar 29, 2017 at 12:28 PM Alex Arslan ***@***.***> wrote: After all, if you just zipped something, why are you unzipping it again? That's a fair point, but if you *haven't* zipped something, why would you be unzipping it at all?

I would have thought that Stefan had been around this business long enough to know that, just because something doesn't make sense, doesn't mean someone won't want to do it.

…

miguelraz · 2017-03-30T05:37:46Z

Work log:
Looked over reviews and pointers, tried to implement suggested fixes.
Thanks a zillion to everybody, this is a cool learning experience.
As always, any critiques/advice/suggestions/welcome.

JackDevine · 2017-06-25T02:47:57Z

base/iterators.jl


-```jldoctest
+"""jldoctest


This is still not quite right, try looking at enumerate on line 35.

JackDevine · 2017-06-25T02:50:27Z

test/iterators.jl

@@ -22,6 +22,22 @@ let z = zip(1:2, 3:4, 5:6)
    @test eltype(z) == Tuple{Int,Int,Int}
 end

+# unzip


Isn't this a good place to use @testset?

JackDevine · 2017-06-25T03:15:28Z

base/iterators.jl

@@ -240,6 +241,34 @@ end
 iteratorsize{I1,I2}(::Type{Zip{I1,I2}}) = zip_iteratorsize(iteratorsize(I1),iteratorsize(I2))
 iteratoreltype{I1,I2}(::Type{Zip{I1,I2}}) = and_iteratoreltype(iteratoreltype(I1),iteratoreltype(I2))

+# unzip
+"""
+


I would have thought that unzip would need it's own method name, something like unzip(iter::AbstractZipIterator). Also, I think that you need to add unzip to exports.jl.

vtjnash · 2021-04-08T22:53:16Z

Replaced by #33515 (@StefanKarpinski there's a couple tests here, if you don't want to write your own 😜)

kshyatt added the needs tests Unit tests are required for this change label Mar 28, 2017

miguelraz added 3 commits March 28, 2017 16:18

Merge pull request #1 from JuliaLang/master

a1ebd47

update julia fork

Merge remote-tracking branch 'miguelraz/patch-3'

9505283

adding tests to unzip

13ac9df

miguelraz mentioned this pull request Mar 28, 2017

adding tests to unzip #21212

Closed

miguelraz added 3 commits March 28, 2017 17:15

trying to push test to patch-3

e7fab27

Merge branch 'master' into patch-3

6b3a9de

fixed typo in test

5508090

Corrected zip and unzip as inverses of each other in docs

e4c7515

fredrikekre reviewed Mar 29, 2017

View reviewed changes

ararslan reviewed Mar 29, 2017

View reviewed changes

miguelraz added 3 commits March 30, 2017 00:28

fixing tests to support systems with different Int

75537c1

unzip test should return a tuple

c9ea231

docs fixing and example fixing

2b98ccc

JackDevine reviewed Jun 25, 2017

View reviewed changes

base/iterators.jl

```jldoctest

"""jldoctest

Copy link

Contributor

JackDevine Jun 25, 2017

Choose a reason for hiding this comment

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

This is still not quite right, try looking at enumerate on line 35.

JackDevine reviewed Jun 25, 2017

View reviewed changes

vtjnash closed this Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP/ RFC/First PR: Add `unzip` to base.iterators.jl #21208

WIP/ RFC/First PR: Add `unzip` to base.iterators.jl #21208

miguelraz commented Mar 28, 2017

JeffBezanson commented Mar 28, 2017 •

edited

Loading

ararslan commented Mar 28, 2017 •

edited

Loading

miguelraz commented Mar 28, 2017 •

edited

Loading

miguelraz commented Mar 28, 2017

ararslan commented Mar 28, 2017 •

edited

Loading

miguelraz commented Mar 28, 2017 •

edited

Loading

StefanKarpinski commented Mar 29, 2017 •

edited

Loading

StefanKarpinski commented Mar 29, 2017

miguelraz commented Mar 29, 2017

fredrikekre Mar 29, 2017

miguelraz Mar 30, 2017

fredrikekre Mar 29, 2017

miguelraz Mar 30, 2017

fredrikekre Mar 29, 2017

miguelraz Mar 30, 2017

fredrikekre Mar 30, 2017

martinholters commented Mar 29, 2017

StefanKarpinski commented Mar 29, 2017

ararslan Mar 29, 2017

miguelraz Mar 30, 2017

ararslan Mar 29, 2017

miguelraz Mar 30, 2017

ararslan commented Mar 29, 2017

dmbates commented Mar 29, 2017 via email

miguelraz commented Mar 30, 2017

JackDevine Jun 25, 2017

JackDevine Jun 25, 2017

JackDevine Jun 25, 2017

vtjnash commented Apr 8, 2021

WIP/ RFC/First PR: Add unzip to base.iterators.jl #21208

WIP/ RFC/First PR: Add unzip to base.iterators.jl #21208

Conversation

miguelraz commented Mar 28, 2017

JeffBezanson commented Mar 28, 2017 • edited Loading

ararslan commented Mar 28, 2017 • edited Loading

miguelraz commented Mar 28, 2017 • edited Loading

miguelraz commented Mar 28, 2017

ararslan commented Mar 28, 2017 • edited Loading

miguelraz commented Mar 28, 2017 • edited Loading

StefanKarpinski commented Mar 29, 2017 • edited Loading

StefanKarpinski commented Mar 29, 2017

miguelraz commented Mar 29, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinholters commented Mar 29, 2017

StefanKarpinski commented Mar 29, 2017

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

ararslan commented Mar 29, 2017

dmbates commented Mar 29, 2017 via email

miguelraz commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Apr 8, 2021

WIP/ RFC/First PR: Add `unzip` to base.iterators.jl #21208

WIP/ RFC/First PR: Add `unzip` to base.iterators.jl #21208

JeffBezanson commented Mar 28, 2017 •

edited

Loading

ararslan commented Mar 28, 2017 •

edited

Loading

miguelraz commented Mar 28, 2017 •

edited

Loading

ararslan commented Mar 28, 2017 •

edited

Loading

miguelraz commented Mar 28, 2017 •

edited

Loading

StefanKarpinski commented Mar 29, 2017 •

edited

Loading