using ranges to filter side by side vectors while remaining readable

Question

So imagine i have 2 vectors (for reasons to do with the framework i am working in):

std::vector<bool> valid = {false, true, true, false};
std::vector<int> percentages = {2, 50, 3, 100};

These vectors represent a batteries validity and the percentage charge. I want to find the minimum charge of the valid batteries. To do this with ranges, the best i can come up with is:

auto min = std::get<1>(
    std::ranges::min(
        std::views::zip(valid, percentages) | 
        std::views::filter([](const auto& vals) {return std::get<0>(vals);}), 
    [](const auto& vals0, const auto& vals1) {
    return std::get<1>(vals0) < std::get<1>(vals1);
}));

Now this works but it is nearly impossible to read. So the for loop variety would be something like:

int min_percentage = 100;
for (const auto& [is_valid, percentage] : std::views::zip(valid, percentages)) {
    if (is_valid) {
        min_percentage = std::min(min_percentage, percentage);
    }
}

Which works just fine as well.

The second one is objectively better. So the question is, is there a way to write this with ranges whilst keeping the code readable?

Barry · Accepted Answer · 2023-03-16T16:48:40.963

In general, the reason that this is poorly readable:

auto min = std::get<1>(
    std::ranges::min(
        std::views::zip(valid, percentages) | 
        std::views::filter([](const auto& vals) {return std::get<0>(vals);}), 
    [](const auto& vals0, const auto& vals1) {
    return std::get<1>(vals0) < std::get<1>(vals1);
}));

is that it's the wrong algorithm - you want to find the minimum percentage but instead the algorithm you're doing is finding the minimum pair of (valid, percentage) by percentage.

Simply restructuring to the correct algorithm (the minimum percentage) is a big improvement:

auto min = 
    std::ranges::min(
        std::views::zip(valid, percentages)
        | std::views::filter([](const auto& vals) {return std::get<0>(vals);})
        | std::views::values);

And there's a proposal in flight (P2769) to let you write that lambda as std::ranges::get_key (or std::ranges::get_element<0>).

Note also that the predicate you passed into min wasn't strictly necessary - the default < would already do the right thing, since all of the tuples in that range are now (true, x), so the minimum value there is, by definition, the one with the minimum x.

This is one of those cases where it's like we have a missing algorithm:

ranges::min(
    views::zip(valid, percentages)
    | views::filter_map([](auto&& e) -> optional<int> {
        auto& [valid, perc] = e;
        if (valid) {
            return perc;
        } else {
            return nullopt;
        }
    })
);

Where filter_map takes a T -> optional<U> and yields a range of U.

In this case though, you can kind of cheat - the exact same algorithm you wrote with your for loop you can accomplish with a transform: you're mapping invalid batteries to 100:

ranges::min(
    views::zip(valid, percentages)
    | views::transform([](auto&& e) {
        auto& [valid, perc] = e;
        return valid ? perc : 100;
    })
);

or, more directly:

ranges::min(
    views::zip_transform([](bool valid, int perc){
        return valid ? perc : 100;
    }, valid, percentages)
);

This has a different semantic in the case where you have no valid batteries (you get a well-defined 100 instead of UB). Although your loop is still well-defined even in the case where there are no batteries (you get 100) whereas here the empty case is still UB.

@T.C. Indeed - although would probably read nicer if I could put the lambda last. — Barry, Mar 16 '23 at 16:59
its annoying that `zip_transform`'s last argument can't be the lambda. Even more so for the equivalent `std::optional` method (i.e. this is applicative application). Kind of destroys readability if you inline a lambda and it's not the last argument. — Tom Huntington, Mar 16 '23 at 21:35
also, lacking `views::filter_map` is very inconvenient because it turns into `transform(...) | filter(std::identity{}) | transform([](std::optional o){return *o;})`. You end up resorting to a hack like "mapping invalid batteries to 100" — Tom Huntington, Mar 16 '23 at 21:46
The `zip_transform` method seems the best, but it does obfuscate what is happening a little — Fantastic Mr Fox, Mar 16 '23 at 23:42

score 3 · Answer 2 · answered Mar 16 '23 at 08:38

I think it's pretty readible if you name the lambdas:

auto is_valid = [](auto const& zipped) { return std::get<0>(zipped); };

auto min = std::ranges::min(
    std::views::zip(valid, percentages) |
    std::views::filter(is_valid) |
    std::views::values
);

https://godbolt.org/z/95T8xsq68

score 1 · Answer 3 · answered Mar 16 '23 at 08:11

1

What about:

namespace rv=std::views;
auto min = std::ranges::min(
    rv::zip(valid, percentages) | 
    rv::filter([](const auto& x)
        {return std::get<0>(x);}) |
    rv::values);

Equivalently you can use rv::elements<1> instead of rv::values.

answered Mar 16 '23 at 08:11

Red.Wave

2,790
11
17

ChrisB · Answer 4 · 2023-03-16T16:24:24.317

Ideally, we could just pass std::get<0> as the filter function.

Unfortunately, the compiler fails to deduce the overload, so we have to help it out with a lambda. Since this is a common issue (How do I specify a pointer to an overloaded function?), having a helper macro for it in your project might be reasonable:

#define AS_LAMBDA(...) [&](auto&&... args)                          \
    -> decltype(__VA_ARGS__(std::forward<decltype(args)>(args)...)) \
{                                                                   \
    return __VA_ARGS__(std::forward<decltype(args)>(args)...);      \
}

This makes the code look as follows:

auto min = std::ranges::min(
   std::views::zip(valid, percentages) |
   std::views::filter(AS_LAMBDA(std::get<0>)) |
   std::views::elements<1>
);

https://godbolt.org/z/Gaa1ErdGG

using ranges to filter side by side vectors while remaining readable

4 Answers4