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 variadic cartesianProduct #1120

Merged
merged 5 commits into from
Feb 7, 2013
Merged

Conversation

quickfur
Copy link
Member

@quickfur quickfur commented Feb 7, 2013

This pull request extends cartesianProduct to 3 or more arguments.

I'm running into a bit of a snag with the unittests, though. Currently I've commented out the last unittest because it runs out of memory on my machine. Can somebody help me find out what's eating up so much memory? Does the cartesianProduct implementation need some tweaking to reduce its memory footprint?

@andralex
Copy link
Member

andralex commented Feb 7, 2013

Would be great to have a unittest and an example showing what sequence gets generated. For example, showcase the entire cartesianProduct([1, 2, 3], ["a", "b", "c"], ["x", "y", "z"]).

@andralex
Copy link
Member

andralex commented Feb 7, 2013

BTW I think this is outstanding work. An article should be dedicated to it.

Use same code in unittest (it's good to make sure examples actually
work! :-P)
@quickfur
Copy link
Member Author

quickfur commented Feb 7, 2013

Hmph, why is the autotester randomly failing in std.parallelism? I didn't touch that code.

tuple(3, 'c', "x"), tuple(3, 'c', "y"), tuple(3, 'c', "z")
];

foreach (e; expected)
Copy link
Member

Choose a reason for hiding this comment

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

Something deterministic and compared to equal would be better. It's not just the totality of the cartesian product that's important, but also the order in which the space is explored. No?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mathematically speaking, the cartesian product is unordered.

It's easy to check for the exact order produced by cartesianProduct, of course, but then it would be tied down to this particular implementation. You wouldn't be able to, for example, implement an improved cartesianProduct that permutes the order of arguments in order to, say, support a product of n input ranges with n forward ranges in any order (and reorders the output so that they match the original order). Such reordering would allow us to further relax the requirements on the ranges.

IOW, you wouldn't be able to change the evaluation schedule.

Copy link
Member

Choose a reason for hiding this comment

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

I understand and agree with all of the above, but I think the primary use of this function is to explore e.g. experiment parameter spaces or vector/matrix/tensor operations etc, and it's important to have a good understanding of the schedule. In that regard the function is a misnomer because order does matter. Second, the right way to change the iteration schedule is via additional parameters or different functions. If you want we can hold off on this until we come up with a design for iteration schedules. But I believe this design is good as is (as long as schedule is fixed) and can be later followed by extended schedules.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@braddr
Copy link
Member

braddr commented Feb 7, 2013

@quickfur, misplaced blame. The auto-tester doesn't randomly fail tests, the TESTS randomly fail.

@quickfur
Copy link
Member Author

quickfur commented Feb 7, 2013

:) OK, point taken. But it would be nice to know why these tests are randomly failing.

andralex added a commit that referenced this pull request Feb 7, 2013
Implement variadic cartesianProduct
@andralex andralex merged commit eec793e into dlang:master Feb 7, 2013
@andralex
Copy link
Member

andralex commented Feb 7, 2013

outstanding

@quickfur quickfur deleted the multi_cartprod branch February 7, 2013 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants