Skip to content

Commit

Permalink
fix: Fix an issue accumulating aggregated metric values. (#149)
Browse files Browse the repository at this point in the history
The issue was caused by the lazy evaluation of `.map()` when
collecting into an `Option`. This is now done explicitly.
  • Loading branch information
jetuk authored Mar 26, 2024
1 parent 2e38724 commit 7a0a969
Showing 1 changed file with 21 additions and 5 deletions.
26 changes: 21 additions & 5 deletions pywr-core/src/recorders/metric_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,27 @@ impl MetricSet {
.as_mut()
.expect("Aggregation state expected for metric set with aggregator!");

let agg_values = values
.into_iter()
.zip(aggregation_states.iter_mut())
.map(|(value, current_state)| aggregator.append_value(current_state, value))
.collect::<Option<Vec<_>>>();
// Collect any aggregated values. This will remain empty if the aggregator yields
// no values. However, if there are values we will expect the same number of aggregated
// values as the input values / metrics.
let mut agg_values = Vec::with_capacity(values.len());
// Use a for loop instead of using an iterator because we need to execute the
// `append_value` method on all aggregators.
for (value, current_state) in values.iter().zip(aggregation_states.iter_mut()) {
if let Some(agg_value) = aggregator.append_value(current_state, *value) {
agg_values.push(agg_value);
}
}

let agg_values = if agg_values.is_empty() {
None
} else if agg_values.len() == values.len() {
Some(agg_values)
} else {
// This should never happen because the aggregator should either yield no values
// or the same number of values as the input metrics.
unreachable!("Some values were aggregated and some were not!");
};

internal_state.current_values = agg_values;
} else {
Expand Down

0 comments on commit 7a0a969

Please sign in to comment.