feat: Improve the metric set implementation. #77
Conversation
This now includes nested aggregation, hooking up the metric sets to the model and use by the CSV recorder. There's still some WIP but a general review would be good at this point.
pywr-core/src/model.rs
Outdated
| mut states, | ||
| mut parameter_internal_states, | ||
| mut metric_set_internal_states, | ||
| mut recorder_internal_states, |
There was a problem hiding this comment.
Might simplify things to combine the states into a single object, if this is feasible?
There was a problem hiding this comment.
Yes, this is something that needs doing. I was planning to tidy it up after this PR
| @@ -42,14 +42,10 @@ impl AggregationPeriod { | |||
| while current_date < end_date { | |||
| // This should be safe to unwrap as it will always create a valid date unless | |||
| // we are at the limit of dates that are representable. | |||
There was a problem hiding this comment.
This comment is old and should be moved to the start_of_next_period function.
| /// Compute the final aggregation value from the current state. | ||
| /// | ||
| /// This will also compute the final aggregation value from the child aggregators if any exists. | ||
| /// This includes aggregation calculations over partial or unfinished periods. |
There was a problem hiding this comment.
Should this be a user setting?
There was a problem hiding this comment.
I'll add this as a new issue for the sake of getting this PR done.
| /// | ||
| /// A metric set can optionally have an aggregator, which will apply an aggregation function | ||
| /// over metrics set. If the aggregator has a defined frequency then the aggregation will result | ||
| /// in multiple values (i.e. per each period implied by the frequency). |
There was a problem hiding this comment.
I think it would be good to have a doc text model here that shows how the nest aggregation works
There was a problem hiding this comment.
I wonder if this would be better as an extended section in the proposed book? I am tempted to leave it for now, but then also need to ensure we write that documentation.
|
I've fixed the out-of-date comment. This could be merged now, or we can address wider documentation issue first (at the expense of delaying the merge). |
This now includes nested aggregation, hooking up the metric sets to the model and use by the CSV recorder.
There's still some WIP but a general review would be good at this point.