tcbrindle / flux

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

BUG: `flux::scan` broken #152

Closed codereport closed 6 months ago

codereport commented 6 months ago

Hey @tcbrindle,

There is an issue with the flux::scan function. It seems as if you are initializing the state to zero, instead of applying the binary operation to the first two elements. It means anything that doesn't have 0 as the "identity" element will break. https://flux.godbolt.org/z/8YK6aErh5

auto iota_scan(int n, auto f) {
    return flux::iota(1, n + 1).scan(f).template to<std::vector>();
}

int main() {
    fmt::print("{}\n", iota_scan(5, std::plus{}));        // ✅ [1, 3, 6, 10, 15]
    fmt::print("{}\n", iota_scan(5, std::multiplies{}));  // ❌ [0, 0, 0, 0, 0]
    fmt::print("{}\n", iota_scan(5, std::ranges::max));   // ✅ [1, 2, 3, 4, 5]
    fmt::print("{}\n", iota_scan(5, std::ranges::min));   // ❌ [0, 0, 0, 0, 0]
}

Technically the max scan would break as well if you had negative numbers.

tcbrindle commented 6 months ago

Hey Conor, thanks for the bug report. In this case however, it's not a bug (as such), but maybe something I need to be more clear about :)

The flux::scan adaptor takes a binary function and an optional initial value -- if the latter is not supplied, it uses a default-constructed object of the sequence's value type, which is 0 in this case.

If you don't want to provide an initial value, there's scan_first() which uses the first element of the sequence as the initial value and applies the operation to subsequent elements. If you use that, it works as expected:

auto iota_scan(int n, auto f) {
    return flux::iota(1, n + 1).scan_first(f).template to<std::vector>();
}

int main() {
    fmt::print("{}\n", iota_scan(5, std::plus{}));        // ✅ [1, 3, 6, 10, 15]
    fmt::print("{}\n", iota_scan(5, std::multiplies{}));  // ✅ [1, 2, 6, 24, 120]
    fmt::print("{}\n", iota_scan(5, flux::cmp::max));   // ✅ [1, 2, 3, 4, 5]
    fmt::print("{}\n", iota_scan(5, flux::cmp::min));   // ✅ [1, 1, 1, 1, 1]
}

https://flux.godbolt.org/z/qqYs3vPvT

The reason for the two functions is that I wanted consistency with fold() and fold_first(), the latter of which also doesn't use an initial value. It's mentioned in the documentation but maybe I need to make the difference more clear somehow.

I hope this explains it!

(Also, FYI, you can now select Flux in the "Libraries" tab in Compiler Explorer, so you don't need to do the #include <really_long_path_to_github> thing any more.)

tcbrindle commented 6 months ago

Also, fmt should now be able to print Flux sequences directly without needing to write them to a vector first -- but it's only in the trunk version of fmt for now, so if you want to try it you'll need to #define FMT_HEADER_ONLY in Compiler Explorer until Victor makes a new release: https://flux.godbolt.org/z/81Tnoc84a

(This should work for all Flux sequences, but it's still new so if you come across any problems please let me know and I'll see if I can fix it.)

codereport commented 6 months ago

Ah, that makes sense. I just need scan_first then! Thanks 🙏