From 201a77295c6844b972df14ab77c50ab15a398bfd Mon Sep 17 00:00:00 2001 From: James Batchelor Date: Mon, 26 Aug 2024 10:47:42 +0100 Subject: [PATCH] response to review comments --- pywr-core/src/recorders/metric_set.rs | 2 +- pywr-schema/src/error.rs | 2 + pywr-schema/src/metric_sets/mod.rs | 78 ++++++++++++++++----------- pywr-schema/src/model.rs | 12 +++-- 4 files changed, 58 insertions(+), 36 deletions(-) diff --git a/pywr-core/src/recorders/metric_set.rs b/pywr-core/src/recorders/metric_set.rs index 350127c2..b269dbee 100644 --- a/pywr-core/src/recorders/metric_set.rs +++ b/pywr-core/src/recorders/metric_set.rs @@ -13,7 +13,7 @@ use std::ops::Deref; /// /// This is used to store the name and attribute of the metric so that it can be output in /// a context that is relevant to the originating schema, and therefore more meaningful to the user. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct OutputMetric { name: String, attribute: String, diff --git a/pywr-schema/src/error.rs b/pywr-schema/src/error.rs index 530796ef..f9714231 100644 --- a/pywr-schema/src/error.rs +++ b/pywr-schema/src/error.rs @@ -60,6 +60,8 @@ pub enum SchemaError { LiteralConstantOutputNotSupported, #[error("Chrono out of range error: {0}")] OutOfRange(#[from] chrono::OutOfRange), + #[error("The metric set with name '{0}' contains no metrics")] + EmptyMetricSet(String), } #[cfg(feature = "core")] diff --git a/pywr-schema/src/metric_sets/mod.rs b/pywr-schema/src/metric_sets/mod.rs index ac361c97..5ab7b557 100644 --- a/pywr-schema/src/metric_sets/mod.rs +++ b/pywr-schema/src/metric_sets/mod.rs @@ -1,8 +1,11 @@ #[cfg(feature = "core")] use crate::error::SchemaError; -use crate::metric::Metric; #[cfg(feature = "core")] use crate::model::LoadArgs; +use crate::{ + metric::Metric, + parameters::{Parameter, PythonReturnType}, +}; use pywr_schema_macros::PywrVisitPaths; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -81,8 +84,12 @@ impl From for pywr_core::recorders::Aggregator { } } +/// Filters that allow multiple metrics to be added to a metric set. +/// +/// The filters allow the default metrics for all nodes and/or parameters in a model +/// to be added to a metric set. #[derive(Deserialize, Serialize, Clone, JsonSchema, Default)] -struct MetricSetFilters { +pub struct MetricSetFilters { #[serde(default)] all_nodes: bool, #[serde(default)] @@ -109,6 +116,14 @@ impl MetricSetFilters { if self.all_parameters { if let Some(parameters) = args.schema.parameters.as_ref() { for parameter in parameters.iter() { + // Skip Python parameters that return multiple values as the type or keys of these values is not + // known at this point. + if let Parameter::Python(param) = parameter { + if matches!(param.return_type, PythonReturnType::Dict) { + continue; + } + } + metrics.push(Metric::Parameter(ParameterReference::new( parameter.name().to_string(), None, @@ -126,13 +141,16 @@ impl MetricSetFilters { /// 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). +/// +/// Metrics added by the filters will be appended to any metrics specified for the metric attribute, +/// if they are not a duplication. #[derive(Deserialize, Serialize, Clone, JsonSchema)] pub struct MetricSet { pub name: String, - pub metrics: Vec, + pub metrics: Option>, pub aggregator: Option, #[serde(default)] - filters: MetricSetFilters, + pub filters: MetricSetFilters, } impl MetricSet { @@ -140,40 +158,38 @@ impl MetricSet { pub fn add_to_model(&self, network: &mut pywr_core::network::Network, args: &LoadArgs) -> Result<(), SchemaError> { use pywr_core::recorders::OutputMetric; - let mut metrics: Vec = self - .metrics - .iter() - //.chain(additional_metric.iter()) - .map(|m| m.load_as_output(network, args)) - .collect::>()?; - - if let Some(additional_metrics) = self.filters.create_metrics(args) { - for m in additional_metrics.iter() { - match m { - Metric::Node(n) => { - if !self.metrics.iter().any(|m| match m { - Metric::Node(n2) => n2.name == n.name, - _ => false, - }) { - metrics.push(m.load_as_output(network, args)?); + let output_metrics = match self.metrics { + Some(ref metrics) => { + let mut output_metrics: Vec = metrics + .iter() + .map(|m| m.load_as_output(network, args)) + .collect::>()?; + + if let Some(additional_metrics) = self.filters.create_metrics(args) { + for m in additional_metrics.iter() { + let output_metric = m.load_as_output(network, args)?; + if !output_metrics.contains(&output_metric) { + output_metrics.push(output_metric); } } - Metric::Parameter(p) => { - if !self.metrics.iter().any(|m| match m { - Metric::Parameter(p2) => p2.name == p.name, - _ => false, - }) { - metrics.push(m.load_as_output(network, args)?); - } - } - _ => {} } + output_metrics } - } + None => { + if let Some(metrics) = self.filters.create_metrics(args) { + metrics + .iter() + .map(|m| m.load_as_output(network, args)) + .collect::>()? + } else { + return Err(SchemaError::EmptyMetricSet(self.name.clone())); + } + } + }; let aggregator = self.aggregator.clone().map(|a| a.into()); - let metric_set = pywr_core::recorders::MetricSet::new(&self.name, aggregator, metrics); + let metric_set = pywr_core::recorders::MetricSet::new(&self.name, aggregator, output_metrics); let _ = network.add_metric_set(metric_set)?; Ok(()) diff --git a/pywr-schema/src/model.rs b/pywr-schema/src/model.rs index b1bfda0e..42ddf23e 100644 --- a/pywr-schema/src/model.rs +++ b/pywr-schema/src/model.rs @@ -220,8 +220,10 @@ impl VisitMetrics for PywrNetwork { if let Some(metric_sets) = &self.metric_sets { for metric_set in metric_sets { - for metric in &metric_set.metrics { - visitor(metric); + if let Some(metrics) = &metric_set.metrics { + for metric in metrics { + visitor(metric); + } } } } @@ -238,8 +240,10 @@ impl VisitMetrics for PywrNetwork { if let Some(metric_sets) = &mut self.metric_sets { for metric_set in metric_sets { - for metric in metric_set.metrics.iter_mut() { - visitor(metric); + if let Some(metrics) = &mut metric_set.metrics { + for metric in metrics { + visitor(metric); + } } } }