tcbrindle / flux

A C++20 library for sequence-orientated programming
https://tristanbrindle.com/flux/
Boost Software License 1.0
472 stars 29 forks source link

Implement internal iteration for `cartesian_product` #92

Closed brycelelbach closed 1 year ago

brycelelbach commented 1 year ago

This PR implements internal iteration for cartesian_product[_with], which enables compiler vectorization of multidimensional iteration patterns constructed with cartesian_product[_with] (such as this one).

Core tasks:

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 88.63% and project coverage change: -0.09% :warning:

Comparison is base (1864f09) 97.67% compared to head (0d3116f) 97.58%. Report is 3 commits behind head on main.

:exclamation: Current head 0d3116f differs from pull request most recent head abba492. Consider uploading reports for the commit abba492 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #92 +/- ## ========================================== - Coverage 97.67% 97.58% -0.09% ========================================== Files 66 66 Lines 2236 2278 +42 ========================================== + Hits 2184 2223 +39 - Misses 52 55 +3 ``` | [Files Changed](https://app.codecov.io/gh/tcbrindle/flux/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tristan+Brindle) | Coverage Δ | | |---|---|---| | [include/flux/core/numeric.hpp](https://app.codecov.io/gh/tcbrindle/flux/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tristan+Brindle#diff-aW5jbHVkZS9mbHV4L2NvcmUvbnVtZXJpYy5ocHA=) | `83.33% <66.66%> (-6.67%)` | :arrow_down: | | [include/flux/op/cartesian\_product.hpp](https://app.codecov.io/gh/tcbrindle/flux/pull/92?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tristan+Brindle#diff-aW5jbHVkZS9mbHV4L29wL2NhcnRlc2lhbl9wcm9kdWN0LmhwcA==) | `98.87% <96.87%> (-1.13%)` | :arrow_down: | ... and [11 files with indirect coverage changes](https://app.codecov.io/gh/tcbrindle/flux/pull/92/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Tristan+Brindle)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tcbrindle commented 1 year ago

Thanks very much for the drive-by fixes, they look great!

In for_each_while_impl, we need to eventually call the predicate with a tuple of elements, not a tuple of cursors. It looks like you are building up a tuple of elements (despite the param name), but constructing the tuple on line 271 doesn't look right to me -- I think it should be element_t rather than cursor_t?

(This happens to work for the particular case of a cartesian product of iotas, because for an iota sequence the element type and the cursor type happen to be the same.)

Other than that I think this looks like a reasonable approach -- I wish the keep_going variable wasn't necessary at every level, but I can't think of a way to get rid of it, and if it doesn't affect vectorisation then I guess we're okay.

brycelelbach commented 1 year ago

In for_each_while_impl, we need to eventually call the predicate with a tuple of elements, not a tuple of cursors. It looks like you are building up a tuple of elements (despite the param name), but constructing the tuple on line 271 doesn't look right to me -- I think it should be element_t rather than cursor_t?

(This happens to work for the particular case of a cartesian product of iotas, because for an iota sequence the element type and the cursor type happen to be the same.)

Ah-ha, I think I suffered from a misunderstanding here. I believe you are correct.

I'm surprised the tests didn't uncover this, but I just realized we don't have any tests that actually call for_each on cartesian_product. I was just testing with my benchmark.

brycelelbach commented 1 year ago

Other than that I think this looks like a reasonable approach -- I wish the keep_going variable wasn't necessary at every level, but I can't think of a way to get rid of it, and if it doesn't affect vectorisation then I guess we're okay.

I was a little bit concerned that the control flow of this would cause issues, but it seems to be fine.

The one idea I had for working around this was to not use flux::for_each_while to iterate each of the underlying sequences, however that seems like a gross violation of the basic principles here. If we did that, it would break more deeply nested internal iteration (for example, cartesian_product(filter(...), ...)).

brycelelbach commented 1 year ago

I'm surprised the tests didn't uncover this, but I just realized we don't have any tests that actually call for_each on cartesian_product. I was just testing with my benchmark.

Ironically, the unpack test that you added does a for_each on cartesian_product, but it's on ints so it doesn't catch this, and I didn't run it before committing anyways.

brycelelbach commented 1 year ago

Ah-ha, I think I suffered from a misunderstanding here. I believe you are correct.

I'm surprised the tests didn't uncover this, but I just realized we don't have any tests that actually call for_each on cartesian_product. I was just testing with my benchmark.

I'm having trouble writing a test that covers this issue. Any ideas?

EDIT: All the cartesian_product tests currently use things that are convertible to the index types, which is probably why I'm not seeing the failures. I can probably engineer something with a mock type.

brycelelbach commented 1 year ago

In for_each_while_impl, we need to eventually call the predicate with a tuple of elements, not a tuple of cursors. It looks like you are building up a tuple of elements (despite the param name), but constructing the tuple on line 271 doesn't look right to me -- I think it should be element_t rather than cursor_t?

(This happens to work for the particular case of a cartesian product of iotas, because for an iota sequence the element type and the cursor type happen to be the same.)

Tests added in a240dcc8d6aa4a1023e28ee94d83d3d68d640d82, fixed in 02e73b3c0d97a414f600dfe5391832cf12a67809.

tcbrindle commented 1 year ago

Ah-ha, I think I suffered from a misunderstanding here. I believe you are correct. I'm surprised the tests didn't uncover this, but I just realized we don't have any tests that actually call for_each on cartesian_product. I was just testing with my benchmark.

I'm having trouble writing a test that covers this issue. Any ideas?

I think what you want is some sequences where the element type and cursor type are distinct and can't be converted to one another? What about something like:

auto seq1 = flux::ints(1).take(3);
auto seq2 = std::array<std::string_view, 2>{"aaa", "bbb"};

auto cart = flux::cartesian_product(seq1, seq2);

auto cur = flux::find_if(cart, flux::unpack([](auto i, auto s) { return i == 2 && s == "bbb"; }));

STATIC_CHECK(std::get<0>(cart[cur]) == 2);
STATIC_CHECK(std::get<1>(cart[cur]) == "bbb");