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

Implement std.algorithm.cartesianProduct #856

Merged
merged 6 commits into from Feb 4, 2013
Merged

Conversation

quickfur
Copy link
Member

As discussed in the forum. This implementation supports infinite ranges as long as they satisfy certain range requirements.

@quickfur
Copy link
Member Author

Yikes! Looks like the autotester is failing on issue 8556 (cf. 8790). :-(

@quickfur
Copy link
Member Author

Fixed coding style, rebased.

@andralex
Copy link
Member

Quick comment - I think we should sleep on getting the infinite forward ranges implementation right; if we commit this the risk is to discourage more general implementations.

The idea for merge on two infinite forward ranges is as follows. Imagine we show the ranges in a plane with r1 horizontally and r2 vertically. Then we first walk the square of side 1. There's only one: tuple(r1[0], r2[0]). Then we walk the square of side 2: (0, 1), (1, 0), and (1, 1). Then we walk the square of side 3: (0, 2), (1, 2), (2, 0), (2, 1), (2, 2). Note that how at each step one index is constant and the other either grows by exactly 1 or it gets reset to 0, thus making the algorithm implementable with forward ranges. The jump from the corner (1, 1) to (0, 2) is a bit special: one range is advanced and the other is reset.

@quickfur
Copy link
Member Author

That's an excellent idea! So basically instead of diagonals, we alternate between vertical and horizontal line segments of increasing lengths, which meet at the diagonal. I'll think about how to implement this.

}
}

// vim:set sw=4 ts=4 expandtab:
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably unrelated line. )

@andralex
Copy link
Member

andralex commented Nov 3, 2012

This is good work. I think it's important that we fix the elusive compiler bug to allow this and many other instances to work. cc @9rnsr

@quickfur
Copy link
Member Author

quickfur commented Nov 5, 2012

Rebased and updated to Andrei's new schedule that only requires forward ranges for the two infinite ranges case. Unittests still do not compile due to the bugs as mentioned in the forum, but I thought I should at least put the code up for review first.

@quickfur
Copy link
Member Author

Just for reference, here's the cause of the current autotester failures:
http://d.puremagic.com/issues/show_bug.cgi?id=8542

@quickfur
Copy link
Member Author

Rebased to prevent from going stale. Since issue 8542 is still open, the autotester will probably still fail, though. :-(

@Poita
Copy link
Contributor

Poita commented Dec 27, 2012

Is there anything you can do to workaround the bug? It would be nice to get this in if the bug is going to take a while to fix.

@quickfur
Copy link
Member Author

Unfortunately I'd have to disable a lot of unittests for that to work. It might be possible to split up the unittests, though. I'll give it a shot when I get home (I'm travelling currently).

@quickfur
Copy link
Member Author

quickfur commented Jan 1, 2013

Rebased, and rewrote unittests to avoid issue 8542. The original unittests are version'd out for future reference, in case we wish to run the old tests again after issue 8542 is fixed.

@quickfur
Copy link
Member Author

quickfur commented Jan 5, 2013

Is std.algorithm getting too big for some of the autotester machines?

http://d.puremagic.com/test-results/pull.ghtml?runid=440874

H. S. Teoh added 6 commits February 3, 2013 17:47
For the case of two infinite ranges, use Andrei's schedule of traversing
right and bottom edges of an increasing square area over the infinite
table of combinations. This allows us to only need to traverse each
range in one direction, so only forward ranges are needed.
Make unittests independent of exact order of pairs produced by
cartesianProduct.

Add unittest for cartesian product of two finite ranges.
@quickfur
Copy link
Member Author

quickfur commented Feb 4, 2013

Rebased to fix merge conflict.

@andralex
Copy link
Member

andralex commented Feb 4, 2013

Looks like we're good to go! Any tests on composing cartesianProduct with itself for taking the product of any number of ranges?

@andralex
Copy link
Member

andralex commented Feb 4, 2013

I'll merge this now anyhow, we can think of adding variadics later.

andralex added a commit that referenced this pull request Feb 4, 2013
Implement std.algorithm.cartesianProduct
@andralex andralex merged commit 8abbbb1 into dlang:master Feb 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants