response to review comments

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,

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")]

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<MetricAggregator> 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,54 +141,55 @@ 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<Metric>,
+    pub metrics: Option<Vec<Metric>>,
     pub aggregator: Option<MetricAggregator>,
     #[serde(default)]
-    filters: MetricSetFilters,
+    pub filters: MetricSetFilters,
 }
 
 impl MetricSet {
     #[cfg(feature = "core")]
     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<OutputMetric> = self
-            .metrics
-            .iter()
-            //.chain(additional_metric.iter())
-            .map(|m| m.load_as_output(network, args))
-            .collect::<Result<_, _>>()?;
-
-        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<OutputMetric> = metrics
+                    .iter()
+                    .map(|m| m.load_as_output(network, args))
+                    .collect::<Result<_, _>>()?;
+
+                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::<Result<_, _>>()?
+                } 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(())

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);
+                    }
                 }
             }
         }