fix: Fix converting dataframe parameters which reference tables. (#305)

Handle conversion of v1.x dataframe parameters that reference tables.

pywr-schema/src/metric.rs

@@ -211,7 +211,7 @@ impl TryFromV1<ParameterValueV1> for Metric {
 
                         Self::Parameter(reference)
                     }
-                    ParameterOrTimeseriesRef::Timeseries(t) => Self::Timeseries(t),
+                    ParameterOrTimeseriesRef::Timeseries(t) => Self::Timeseries(t.ts_ref),
                 }
             }
         };

pywr-schema/src/model.rs

@@ -317,11 +317,12 @@ impl PywrNetwork {
         timeseries.extend(conversion_data.timeseries);
         parameters.extend(conversion_data.parameters);
 
-        // closure to update a parameter ref with a timeseries ref when names match.
+        // Closure to update a parameter ref with a timeseries ref when names match.
+        // We match on the original parameter name because the parameter name may have been changed
         let update_to_ts_ref = &mut |m: &mut Metric| {
             if let Metric::Parameter(p) = m {
-                if let Some(ts_ref) = timeseries_refs.iter().find(|ts| ts.name() == p.name) {
-                    *m = Metric::Timeseries(ts_ref.clone());
+                if let Some(converted_ts_ref) = timeseries_refs.iter().find(|ts| ts.original_parameter_name == p.name) {
+                    *m = Metric::Timeseries(converted_ts_ref.ts_ref.clone());
                 }
             }
         };

pywr-schema/src/parameters/mod.rs

@@ -32,7 +32,7 @@ use crate::error::SchemaError;
 use crate::metric::{Metric, ParameterReference};
 #[cfg(feature = "core")]
 use crate::model::LoadArgs;
-use crate::timeseries::TimeseriesReference;
+use crate::timeseries::ConvertedTimeseriesReference;
 use crate::v1::{ConversionData, TryFromV1, TryIntoV2};
 use crate::visit::{VisitMetrics, VisitPaths};
 pub use aggregated::{AggFunc, AggregatedIndexParameter, AggregatedParameter, IndexAggFunc};
@@ -357,7 +357,7 @@ impl VisitPaths for Parameter {
 pub enum ParameterOrTimeseriesRef {
     // Boxed due to large size difference.
     Parameter(Box<Parameter>),
-    Timeseries(TimeseriesReference),
+    Timeseries(ConvertedTimeseriesReference),
 }
 
 impl From<Parameter> for ParameterOrTimeseriesRef {
@@ -366,8 +366,8 @@ impl From<Parameter> for ParameterOrTimeseriesRef {
     }
 }
 
-impl From<TimeseriesReference> for ParameterOrTimeseriesRef {
-    fn from(t: TimeseriesReference) -> Self {
+impl From<ConvertedTimeseriesReference> for ParameterOrTimeseriesRef {
+    fn from(t: ConvertedTimeseriesReference) -> Self {
         Self::Timeseries(t)
     }
 }
@@ -430,12 +430,14 @@ impl TryFromV1<ParameterV1> for ParameterOrTimeseriesRef {
                 }
                 CoreParameter::Min(p) => Parameter::Min(p.try_into_v2(parent_node, conversion_data)?).into(),
                 CoreParameter::Division(p) => Parameter::Division(p.try_into_v2(parent_node, conversion_data)?).into(),
-                CoreParameter::DataFrame(p) => <DataFrameParameterV1 as TryIntoV2<TimeseriesReference>>::try_into_v2(
-                    p,
-                    parent_node,
-                    conversion_data,
-                )?
-                .into(),
+                CoreParameter::DataFrame(p) => {
+                    <DataFrameParameterV1 as TryIntoV2<ConvertedTimeseriesReference>>::try_into_v2(
+                        p,
+                        parent_node,
+                        conversion_data,
+                    )?
+                    .into()
+                }
                 CoreParameter::Deficit(p) => {
                     return Err(ConversionError::DeprecatedParameter {
                         ty: "DeficitParameter".to_string(),

pywr-schema/src/timeseries/mod.rs

@@ -319,7 +319,18 @@ impl TimeseriesReference {
     }
 }
 
-impl TryFromV1<DataFrameParameterV1> for TimeseriesReference {
+/// Helper struct to convert references to timeseries.
+///
+/// Keeps a reference to the original parameter name and the new timeseries reference. If the
+/// timeseries refers to a table then the original parameter name is no longer required in the
+/// final model, but is needed during conversion to ensure that the table is correctly referenced.
+#[derive(Clone)]
+pub struct ConvertedTimeseriesReference {
+    pub original_parameter_name: String,
+    pub ts_ref: TimeseriesReference,
+}
+
+impl TryFromV1<DataFrameParameterV1> for ConvertedTimeseriesReference {
     type Error = ConversionError;
 
     fn try_from_v1(
@@ -328,6 +339,7 @@ impl TryFromV1<DataFrameParameterV1> for TimeseriesReference {
         conversion_data: &mut ConversionData,
     ) -> Result<Self, Self::Error> {
         let meta: ParameterMeta = v1.meta.into_v2(parent_node, conversion_data);
+        let mut ts_name = meta.name.clone();
 
         if let Some(url) = v1.url {
             // If there is a URL then this entry must be converted into a timeseries
@@ -354,8 +366,10 @@ impl TryFromV1<DataFrameParameterV1> for TimeseriesReference {
             if !conversion_data.timeseries.iter().any(|ts| ts.meta.name == meta.name) {
                 conversion_data.timeseries.push(timeseries);
             }
-        } else if v1.table.is_some() {
-            // Nothing to do here; The table will be converted separately
+        } else if let Some(table) = v1.table {
+            // If this is a reference to a table then we need to point to the table by name, and
+            // ignore the original parameter's name entirely.
+            ts_name = table;
         } else {
             return Err(ConversionError::MissingAttribute {
                 attrs: vec!["url".to_string(), "table".to_string()],
@@ -371,11 +385,10 @@ impl TryFromV1<DataFrameParameterV1> for TimeseriesReference {
             (None, None) => None,
         };
         // The reference that is returned
-        let reference = TimeseriesReference {
-            name: meta.name,
-            columns,
-        };
-
-        Ok(reference)
+        let reference = TimeseriesReference { name: ts_name, columns };
+        Ok(ConvertedTimeseriesReference {
+            original_parameter_name: meta.name,
+            ts_ref: reference,
+        })
     }
 }

pywr-schema/tests/test_models.rs

@@ -139,6 +139,5 @@ fn convert_model(v1_path: &Path, v2_path: &Path) {
 
     let v2_expected_str = fs::read_to_string(v2_path).unwrap();
     let v2_expected: serde_json::Value = serde_json::from_str(&v2_expected_str).unwrap();
-
     assert_eq!(v2_converted, v2_expected);
 }

pywr-schema/tests/v1/timeseries-converted.json

@@ -43,6 +43,23 @@
         "parameters": null,
         "cost": null
       },
+      {
+        "type": "Input",
+        "max_flow": {
+          "type": "Timeseries",
+          "name": "inflow-tbl",
+          "columns": {
+            "type": "Column",
+            "name": "inflow1"
+          }
+        },
+        "meta": {
+          "name": "input3"
+        },
+        "min_flow": null,
+        "parameters": null,
+        "cost": null
+      },
       {
         "type": "Link",
         "max_flow": null,
@@ -82,6 +99,10 @@
         "from_node": "input2",
         "to_node": "link1"
       },
+      {
+        "from_node": "input3",
+        "to_node": "link1"
+      },
       {
         "from_node": "link1",
         "to_node": "output1"

pywr-schema/tests/v1/timeseries.json

@@ -1,59 +1,89 @@
 {
-    "metadata": {
-        "title": "Simple timeseries"
+  "metadata": {
+    "title": "Simple timeseries"
+  },
+  "timestepper": {
+    "start": "2021-01-01",
+    "end": "2021-01-31",
+    "timestep": 1
+  },
+  "nodes": [
+    {
+      "name": "input1",
+      "type": "Input",
+      "max_flow": "inflow"
     },
-    "timestepper": {
-        "start": "2021-01-01",
-        "end": "2021-01-31",
-        "timestep": 1
+    {
+      "name": "input2",
+      "type": "Input",
+      "max_flow": "factored_flow"
     },
-    "nodes": [
-        {
-            "name": "input1",
-            "type": "Input",
-            "max_flow": "inflow"
-        },
-        {
-            "name": "input2",
-            "type": "Input",
-            "max_flow": "factored_flow"
-        },
-        {
-            "name": "link1",
-            "type": "Link"
-        },
-        {
-            "name": "output1",
-            "type": "Output",
-            "max_flow": "demand",
-            "cost": -10
-        }
+    {
+      "name": "input3",
+      "type": "Input",
+      "max_flow": "inflow3"
+    },
+    {
+      "name": "link1",
+      "type": "Link"
+    },
+    {
+      "name": "output1",
+      "type": "Output",
+      "max_flow": "demand",
+      "cost": -10
+    }
+  ],
+  "edges": [
+    [
+      "input1",
+      "link1"
+    ],
+    [
+      "input2",
+      "link1"
     ],
-    "edges": [
-        ["input1", "link1"],
-        ["input2", "link1"],
-        ["link1", "output1"]
+    [
+      "input3",
+      "link1"
     ],
-    "parameters": {
-        "demand": {
-            "type": "constant",
-            "value": 100.0
-        },
-        "inflow": {
-            "type": "dataframe",
-            "url" : "inflow.csv",
-            "parse_dates": true,
-            "dayfirst": true,
-            "index_col": 0,
-            "column": "inflow1"
-        },
-        "factored_flow": {
-            "type": "aggregated",
-            "agg_func":"product",
-            "parameters": [
-                "inflow",
-                0.5
-            ]
-        }
+    [
+      "link1",
+      "output1"
+    ]
+  ],
+  "parameters": {
+    "demand": {
+      "type": "constant",
+      "value": 100.0
+    },
+    "inflow": {
+      "type": "dataframe",
+      "url": "inflow.csv",
+      "parse_dates": true,
+      "dayfirst": true,
+      "index_col": 0,
+      "column": "inflow1"
+    },
+    "inflow3": {
+      "type": "dataframe",
+      "table": "inflow-tbl",
+      "column": "inflow1"
+    },
+    "factored_flow": {
+      "type": "aggregated",
+      "agg_func": "product",
+      "parameters": [
+        "inflow",
+        0.5
+      ]
+    }
+  },
+  "tables": {
+    "inflow-tbl": {
+      "url": "inflow.csv",
+      "parse_dates": true,
+      "dayfirst": true
     }
+  }
 }