_.unzip unit tests not testing documented usage. #1190

Closed
jdalton opened this Issue Jul 6, 2013 · 10 comments

Comments

Projects
None yet
3 participants
@jdalton
Contributor

jdalton commented Jul 6, 2013

The _.unzip unit tests are testing only

_.unzip(array1, array2, array3);

which is not the documented usage.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 6, 2013

Contributor

Looks like it's documented in the source one way and in the documentation another:

  // The inverse operation to `_.zip`. If given an array of pairs it
  // returns an array of the paired elements split into two left and
  // right element arrays, if given an array of triples it returns a
  // three element array and so on. For example, `_.unzip` given
  // `[['a',1],['b',2],['c',3]]` returns the array
  // [['a','b','c'],[1,2,3]].
Contributor

jdalton commented Jul 6, 2013

Looks like it's documented in the source one way and in the documentation another:

  // The inverse operation to `_.zip`. If given an array of pairs it
  // returns an array of the paired elements split into two left and
  // right element arrays, if given an array of triples it returns a
  // three element array and so on. For example, `_.unzip` given
  // `[['a',1],['b',2],['c',3]]` returns the array
  // [['a','b','c'],[1,2,3]].
@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 6, 2013

Contributor

The operation:

 _.unzip(_.zip(['moe', 'larry'], [30, 40], [true, false]));
// produces [[["moe",30,true]],[["larry",40,false]]]
// instead of [['moe', 'larry'], [30, 40], [true, false]]
Contributor

jdalton commented Jul 6, 2013

The operation:

 _.unzip(_.zip(['moe', 'larry'], [30, 40], [true, false]));
// produces [[["moe",30,true]],[["larry",40,false]]]
// instead of [['moe', 'larry'], [30, 40], [true, false]]
@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 6, 2013

Contributor

Looks like this change was introduced in the actual 1.5.0 commit :/

Contributor

jdalton commented Jul 6, 2013

Looks like this change was introduced in the actual 1.5.0 commit :/

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jul 6, 2013

Owner

Not broke, changed. Updated the comment above.

Owner

jashkenas commented Jul 6, 2013

Not broke, changed. Updated the comment above.

@jashkenas jashkenas closed this Jul 6, 2013

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 6, 2013

Contributor

Why the last minute change in functionality?
Now it can't be easily used to unzip a zipped value w/o using apply or referencing each element of the zipped result.
Can you explain what the benefit is? (since it's not what was originally requested, discussed, or implemented)

Contributor

jdalton commented Jul 6, 2013

Why the last minute change in functionality?
Now it can't be easily used to unzip a zipped value w/o using apply or referencing each element of the zipped result.
Can you explain what the benefit is? (since it's not what was originally requested, discussed, or implemented)

@knowtheory

This comment has been minimized.

Show comment
Hide comment
@knowtheory

knowtheory Jul 7, 2013

Collaborator

It's a bit of a bummer that zip and unzip aren't inverse functions. I wish there were some equivalent of Ruby's splat operator to deal with having to do something like this:

res = _.zip(['moe', 'larry'], [30, 40], [true, false]);
_.unzip(res[0], res[1]);

Though, none of the other array functions take multiple arguments and return an array of results, on the other hand.

Collaborator

knowtheory commented Jul 7, 2013

It's a bit of a bummer that zip and unzip aren't inverse functions. I wish there were some equivalent of Ruby's splat operator to deal with having to do something like this:

res = _.zip(['moe', 'larry'], [30, 40], [true, false]);
_.unzip(res[0], res[1]);

Though, none of the other array functions take multiple arguments and return an array of results, on the other hand.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jul 7, 2013

Owner

Because this way, they're parallel. Previously, zip took individual arrays as arguments, and unzip took an array of (somewhat carefully constructed) arrays. Previously, they weren't inverse functions, because you couldn't have passed the output of unzip directly into zip.

This way, they both allow input as individual arguments (as you might want to write it), and can be apply'd in the same way to take in an array of arrays as a singular argument as well.

Owner

jashkenas commented Jul 7, 2013

Because this way, they're parallel. Previously, zip took individual arrays as arguments, and unzip took an array of (somewhat carefully constructed) arrays. Previously, they weren't inverse functions, because you couldn't have passed the output of unzip directly into zip.

This way, they both allow input as individual arguments (as you might want to write it), and can be apply'd in the same way to take in an array of arrays as a singular argument as well.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Jul 7, 2013

Contributor

Wait, now _.zip is an alias for _.unzip:

_.zip(['moe', 'larry', 'curly'], [30, 40, 50], [true, false, false]);
// => [["moe", 30, true], ["larry", 40, false], ["curly", 50, false]]

_.unzip(['moe', 'larry', 'curly'], [30, 40, 50], [true, false, false]);
// => [["moe", 30, true], ["larry", 40, false], ["curly", 50, false]]

The docs for _.unzip (minus sentence with opposite) are interchangeable with _.zip, which means the _.unzip docs are misleading as it states it's the opposite of _.zip and the code is extraneous as it's nothing more than _.zip.

I think it would have made more sense to keep it the way it was originally implemented, possibly extending _.zip to accept _.unzip results.

Contributor

jdalton commented Jul 7, 2013

Wait, now _.zip is an alias for _.unzip:

_.zip(['moe', 'larry', 'curly'], [30, 40, 50], [true, false, false]);
// => [["moe", 30, true], ["larry", 40, false], ["curly", 50, false]]

_.unzip(['moe', 'larry', 'curly'], [30, 40, 50], [true, false, false]);
// => [["moe", 30, true], ["larry", 40, false], ["curly", 50, false]]

The docs for _.unzip (minus sentence with opposite) are interchangeable with _.zip, which means the _.unzip docs are misleading as it states it's the opposite of _.zip and the code is extraneous as it's nothing more than _.zip.

I think it would have made more sense to keep it the way it was originally implemented, possibly extending _.zip to accept _.unzip results.

jashkenas added a commit that referenced this issue Jul 8, 2013

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jul 8, 2013

Owner

Quite so. I guess that the original proposal to add unzip was a bit foolish from the get-go. Ruby, for example, only has a zip, and Python, for another, only implements a zip as well -- for precisely this reason. An index-based-transposition is a transposition, and can be used to go back and forth.

Owner

jashkenas commented Jul 8, 2013

Quite so. I guess that the original proposal to add unzip was a bit foolish from the get-go. Ruby, for example, only has a zip, and Python, for another, only implements a zip as well -- for precisely this reason. An index-based-transposition is a transposition, and can be used to go back and forth.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 9, 2013

Cool. I'm going to go with making _.unzip an alias of _.zip and allow it to consume its results.

var expected = [['moe', 'larry'], [30, 40]];
 _.unzip(_.zip(_.unzip(_.zip(expected))));
// => [['moe', 'larry'], [30, 40]]

ghost commented Jul 9, 2013

Cool. I'm going to go with making _.unzip an alias of _.zip and allow it to consume its results.

var expected = [['moe', 'larry'], [30, 40]];
 _.unzip(_.zip(_.unzip(_.zip(expected))));
// => [['moe', 'larry'], [30, 40]]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment