Is the a better way to do an average of a unordered_multimap key?

Question

I'm searching for get all value of one key in my unordered_multimap, which is actually stacking an int and a value who represent an execution time in nanosecs. I need to get all value to replace the multi keys by one key with an average.

I tried some code, this one is the most valuable actually :

std::unordered_multimap<int, std::chrono::nanoseconds> data;
std::chrono::nanoseconds average;
// *filling my map with value*
for (auto & pair : data){
  auto range = data.equal_range(pair.first);

  for_each (
    range.first,
    range.second,
    [](std::unordered_multimap<int, std::chrono::nanoseconds>::value_type& x){
      average = average + x.second;
    }
  );
  average = average / data.count(pair.first);
  data.erase(pair.first);
  data.insert({pair.first, average});
  }

The error i get error: 'average' is not captured : average = average + x.second;

I don't get the question. Why not just declare `average` at a higher scope? — NathanOliver, Dec 03 '19 at 17:02
`The problem is that average isn't defined out of the for_each loop` - so define average out of the for_each loop. Problem solved! — Klaycon, Dec 03 '19 at 17:02
Like the error says, you didn't capture `average`. Add it to the capture list of the lambda. — NathanOliver, Dec 03 '19 at 17:08
I did change and did the declaration at higher scope, still not working — LeoDevSecOps, Dec 03 '19 at 17:10
I don't understand how to add it to capture list of the lambda, where do i have to put it ? — LeoDevSecOps, Dec 03 '19 at 17:12
`[&average](std::...`. You still have to reset `average` between iterations though, otherwise you'll get wrong results. — Max Langhof, Dec 03 '19 at 17:12
`[](std::unordered_multimap::value_type& x)` -> `[&average](std::unordered_multimap::value_type& x)` — NathanOliver, Dec 03 '19 at 17:13

Caleth · Accepted Answer · 2019-12-03T18:46:01.577

3

Rather than std::for_each, use std::accumulate.

Note that removing entries from a std::unordered_multimap within a ranged-for over it is undefined behaviour. It's safer to fill a different container.

std::unordered_multimap<int, std::chrono::nanoseconds> data;
// *filling my map with value*

std::unordered_multimap<int, std::chrono::nanoseconds> new_data;
for (auto it = data.begin(); it != data.end(); ++it){
  auto range = data.equal_range(it->first);

  auto average = std::accumulate (
    range.first,
    range.second,
    std::chrono::nanoseconds{0},
    [](auto sum, auto & x){
      return sum + x.second;
    }
  ) / std::distance(range.first, range.second);

  new_data.emplace(key, average);
}
data.swap(new_data);

Alternatively, if you have C++17, std::transform_reduce.

  std::transform_reduce(
    range.first,
    range.second,
    std::chrono::nanoseconds{0},
    std::plus<>{},
    [](auto & x) { return x.second; }
  )

edited Dec 03 '19 at 18:46

answered Dec 03 '19 at 17:15

Caleth

52,200
2
44
75

I tried this, but had a lot of errors, can't compile at all – LeoDevSecOps Dec 03 '19 at 17:25
Is `emplace` really always safe? I suppose it cannot cause rehashing since the size doesn't increase, but is the new element's iterator guaranteed to be ordered before `it` returned from `erase`? – walnut Dec 03 '19 at 17:26
That working well, i did change after your edit, all good !! Thank you – LeoDevSecOps Dec 03 '19 at 17:29
@walnut maybe? probably safer to fill a `decltype(data) new_data;` and `data.swap(new_data);` afterwards – Caleth Dec 03 '19 at 17:36
@Caleth Yes, that would have been my suggestion. Also allows dropping the `erase` and using an ordinary iterator loop. According to cppreference only since C++14 is `erase` guaranteed to preserve the order of the iterators and it doesn't say anything about `emplace` in that regard. – walnut Dec 03 '19 at 17:40

Is the a better way to do an average of a unordered_multimap key?

1 Answers1