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

Adding _.combine #168

Merged
merged 4 commits into from Aug 14, 2014

Conversation

@krawaller
Copy link
Contributor

commented Aug 14, 2014

This pull request implements a feature I've been missing; it returns an array of all possible combinations using one element each from the given arrays.

Each element in the return array with therefore contain as many elements as you fed combine, and the number of elements in the return array equals the product of the lengths of the given arrays.

Here's an example:

_.combine([1,2,3],["a","b"],[”foo","bar","baz"]);
// => [ [1,"a","foo"],[1,"a","bar"],[1,"a","baz"],[1,"b","foo"],[1,"b","bar"],[1,"b","baz"],[2,"a","foo"],[2,"a","bar"],[2,"a","baz"],[2,"b","foo"],[2,"b","bar"],[2,"b","baz"]]

This was deemed not quite useful enough for Underscore proper, but might qualify for _.contrib? I use it somewhat often, and as doing it "manually" is messy and verbose, a helper for this functionality really cleans up the code.

Adding _.combine
Adding array builder `combine` that returns an array containing all
possible combinations using one element each from the given arrays.
@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Not too happy with the name though. Perhaps combinations? Or mix?

return _.reduce(slice.call(arguments, 1),function(ret,newarr){
return _.reduce(ret,function(memo,oldi){
return memo.concat(_.reduce(newarr,function(m,newi){
m.push(oldi.concat(newi));

This comment has been minimized.

Copy link
@megawac

megawac Aug 14, 2014

Member

why isnt this a map?

replaced last reduce with map
Innermost reduce call should of course be a map! Thanx @megawac!
@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Good catch, @megawac , thanx! Just as I was feeling all ninja-y over the code. :)

Fixed now!

@fogus

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

I like _.combinations since that opens up a chance for someone to contribute _.permutations as well. :)

return oldi.concat(newi);
}));
},[]);
},_.map(arguments[0],function(i){return [i];}));

This comment has been minimized.

Copy link
@megawac

megawac Aug 14, 2014

Member

whats the reason for this identity map?

This comment has been minimized.

Copy link
@krawaller

krawaller Aug 14, 2014

Author Contributor

It's not identity, note I'm wrapping each element in an array. Subsequent reduces will concat stuff to those!

This comment has been minimized.

Copy link
@megawac

megawac Aug 14, 2014

Member

Untested but it can probably be changed to something like

function combine() {
    return _.reduce(arguments, function(ret, newarr) {
        return _.reduce(ret, function(memo, oldi) {
            return memo.concat(_.map(newarr, function(newi) {
                return oldi.concat(newi);
            }));
        }, []);
    });
}

This comment has been minimized.

Copy link
@megawac

megawac Aug 14, 2014

Member

Ah, Array.of :)

This comment has been minimized.

Copy link
@krawaller

krawaller Aug 14, 2014

Author Contributor

Since oldi won't (necessarily) be an array now, the innermost concat call will fail.

namechange
changed from `combine` to `combinations` as per suggestion from @fogus
@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Good point, @fogus ! Name change pushed.

@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Also, totally not the place or time, but: absolutely adored your book, @fogus ! Excellent work, thanx for sharing! :)

made sure initial array elements are preserved
Code comment from @megawac made me realise that if initial arrays
contained array elements, those were flattened which is not what we
want. This commit fixes that (and adds tests).
fogus added a commit that referenced this pull request Aug 14, 2014

@fogus fogus merged commit e138958 into documentcloud:master Aug 14, 2014

@fogus

This comment has been minimized.

Copy link
Member

commented Aug 14, 2014

Thanks! And thank you for combinations.

@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2014

Had a thought; without reading the specifics, someone might expect a function named combinations to have the signature combinations(arr,n) and return the possible arrays for taking n elements from arr.

Perhaps make the function act that way when called with an array and an integer? Or maybe give my function a different name (combineArrays), and implement combinations acting like described above as a separate function?

@berrytj

This comment has been minimized.

Copy link

commented Aug 18, 2014

The technical term for this is cartesian product, and in python it's itertools.product -- just food for thought WRT naming.

@krawaller

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2014

I think I figured out a neat solution to the API woes; how about changing the signature to allow an optional last argument n, which says how many elements to take from each array?

That way it covers the (presumably) common use case where we want combinations from a single array.

It would also makes working with multiple arrays more flexible. And letting n default to 1 would make it "backwards compatible" to current implementation.

Downside is it kills the otherwise excellent (I facepalmed, should have thought of that myself) naming suggestion from @berrytj . Any ideas what to call this new hybrid between cartesian product and combinations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.