From af2dba7847a3372d797946a7aab198182ed64bf1 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Thu, 12 Dec 2024 10:29:27 +0000 Subject: [PATCH 01/11] Pass flows to functions --- src/process.rs | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/process.rs b/src/process.rs index b8c458a1d..6b42848f5 100644 --- a/src/process.rs +++ b/src/process.rs @@ -330,6 +330,7 @@ fn read_process_pacs_from_iter( iter: I, process_ids: &HashSet>, commodities: &HashMap, Rc>, + flows: &HashMap, Vec>, ) -> Result, Vec>>> where I: Iterator, @@ -337,20 +338,25 @@ where // Keep track of previous PACs so we can check for duplicates let mut pacs = HashSet::new(); - iter.map(|pac| { - let process_id = process_ids.get_id(&pac.process_id)?; - let commodity = commodities.get(pac.commodity_id.as_str()); + let pacs = iter + .map(|pac| { + let process_id = process_ids.get_id(&pac.process_id)?; + let commodity = commodities.get(pac.commodity_id.as_str()); - match commodity { - None => bail!("{} is not a valid commodity ID", &pac.commodity_id), - Some(commodity) => { - ensure!(pacs.insert(pac), "Duplicate PACs found"); + match commodity { + None => bail!("{} is not a valid commodity ID", &pac.commodity_id), + Some(commodity) => { + ensure!(pacs.insert(pac), "Duplicate PACs found"); - Ok((process_id, Rc::clone(commodity))) + Ok((process_id, Rc::clone(commodity))) + } } - } - }) - .process_results(|iter| iter.into_group_map()) + }) + .process_results(|iter| iter.into_group_map()); + + // Check that all PACs are inputs or outputs + + pacs } /// Read process Primary Activity Commodities (PACs) from the specified model directory. @@ -364,10 +370,11 @@ fn read_process_pacs( model_dir: &Path, process_ids: &HashSet>, commodities: &HashMap, Rc>, + flows: &HashMap, Vec>, ) -> Result, Vec>>> { let file_path = model_dir.join(PROCESS_PACS_FILE_NAME); let process_pacs_csv = read_csv(&file_path)?; - read_process_pacs_from_iter(process_pacs_csv, process_ids, commodities) + read_process_pacs_from_iter(process_pacs_csv, process_ids, commodities, flows) .with_context(|| input_err_msg(&file_path)) } @@ -398,7 +405,7 @@ pub fn read_processes( let mut availabilities = read_process_availabilities(model_dir, &process_ids, time_slice_info)?; let file_path = model_dir.join(PROCESS_FLOWS_FILE_NAME); let mut flows = read_csv_grouped_by_id(&file_path, &process_ids)?; - let mut pacs = read_process_pacs(model_dir, &process_ids, commodities)?; + let mut pacs = read_process_pacs(model_dir, &process_ids, commodities, &flows)?; let mut parameters = read_process_parameters(model_dir, &process_ids, year_range)?; let file_path = model_dir.join(PROCESS_REGIONS_FILE_NAME); let mut regions = From d67ed66df0bd1e4ddce6c28bf7abe226add0c09d Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Thu, 12 Dec 2024 10:50:10 +0000 Subject: [PATCH 02/11] Add empty flows to tests --- src/process.rs | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/process.rs b/src/process.rs index 6b42848f5..dae2972b0 100644 --- a/src/process.rs +++ b/src/process.rs @@ -724,6 +724,7 @@ mod tests { #[test] fn test_read_process_pacs_from_iter() { + // Prepare test data let process_ids = ["id1".into(), "id2".into()].into_iter().collect(); let commodities = ["commodity1", "commodity2"] .into_iter() @@ -740,6 +741,7 @@ mod tests { (Rc::clone(&commodity.id), commodity.into()) }) .collect(); + let flows: HashMap, Vec> = HashMap::new(); // duplicate PAC let pac = ProcessPAC { @@ -747,17 +749,27 @@ mod tests { commodity_id: "commodity1".into(), }; let pacs = [pac.clone(), pac]; - assert!(read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities).is_err()); + assert!( + read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities, &flows) + .is_err() + ); // invalid commodity ID let bad_pac = ProcessPAC { process_id: "id1".into(), commodity_id: "other_commodity".into(), }; - assert!( - read_process_pacs_from_iter([bad_pac].into_iter(), &process_ids, &commodities).is_err() - ); + assert!(read_process_pacs_from_iter( + [bad_pac].into_iter(), + &process_ids, + &commodities, + &flows + ) + .is_err()); + + // Invalid flows + // Valid let pacs = [ ProcessPAC { process_id: "id1".into(), @@ -794,7 +806,8 @@ mod tests { .into_iter() .collect(); assert!( - read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities).unwrap() + read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities, &flows) + .unwrap() == expected ); } From 47b47ef387999eb7ce8e17c922ef62142e882a29 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Thu, 12 Dec 2024 15:46:37 +0000 Subject: [PATCH 03/11] Add check for PAC flow sign --- src/process.rs | 45 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/process.rs b/src/process.rs index dae2972b0..328a562e8 100644 --- a/src/process.rs +++ b/src/process.rs @@ -338,25 +338,58 @@ where // Keep track of previous PACs so we can check for duplicates let mut pacs = HashSet::new(); - let pacs = iter + // Build hashmap of process ID to PAC commodities + let _pacs = iter .map(|pac| { let process_id = process_ids.get_id(&pac.process_id)?; let commodity = commodities.get(pac.commodity_id.as_str()); + // Check that commodity is valid and not a duplicate match commodity { None => bail!("{} is not a valid commodity ID", &pac.commodity_id), Some(commodity) => { ensure!(pacs.insert(pac), "Duplicate PACs found"); - Ok((process_id, Rc::clone(commodity))) } } }) - .process_results(|iter| iter.into_group_map()); - - // Check that all PACs are inputs or outputs + .process_results(|iter| iter.into_group_map())?; + + // Check that PACs for each process are either all inputs or all outputs + for (process_id, pacs) in _pacs.iter() { + let flows = flows.get(process_id).unwrap(); + + let mut flow_sign: Option = None; // False for inputs, true for outputs + for pac in pacs.iter() { + // Find the flow associated with the PAC + let flow = flows + .iter() + .find(|item| *item.commodity_id.as_str() == *pac.id) + .with_context(|| { + format!( + "PAC {} for process {} must have an associated flow", + pac.id, process_id + ) + })?; + + // Check that flow sign is consistent + let current_flow_sign = flow.flow > 0.0; + match flow_sign { + None => { + flow_sign = Some(current_flow_sign); + } + Some(existing_flow_type) => { + ensure!( + existing_flow_type == current_flow_sign, + "PACs for process {} are a mix of inputs and outputs", + process_id + ); + } + }; + } + } - pacs + Ok(_pacs) } /// Read process Primary Activity Commodities (PACs) from the specified model directory. From 0102f20b1f1759935afc5b208e8390c4aa693dc8 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Thu, 12 Dec 2024 16:34:47 +0000 Subject: [PATCH 04/11] Rename objects --- src/process.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/process.rs b/src/process.rs index 328a562e8..d7391a456 100644 --- a/src/process.rs +++ b/src/process.rs @@ -336,10 +336,10 @@ where I: Iterator, { // Keep track of previous PACs so we can check for duplicates - let mut pacs = HashSet::new(); + let mut existing_pacs = HashSet::new(); // Build hashmap of process ID to PAC commodities - let _pacs = iter + let pacs = iter .map(|pac| { let process_id = process_ids.get_id(&pac.process_id)?; let commodity = commodities.get(pac.commodity_id.as_str()); @@ -348,7 +348,7 @@ where match commodity { None => bail!("{} is not a valid commodity ID", &pac.commodity_id), Some(commodity) => { - ensure!(pacs.insert(pac), "Duplicate PACs found"); + ensure!(existing_pacs.insert(pac), "Duplicate PACs found"); Ok((process_id, Rc::clone(commodity))) } } @@ -356,7 +356,7 @@ where .process_results(|iter| iter.into_group_map())?; // Check that PACs for each process are either all inputs or all outputs - for (process_id, pacs) in _pacs.iter() { + for (process_id, pacs) in pacs.iter() { let flows = flows.get(process_id).unwrap(); let mut flow_sign: Option = None; // False for inputs, true for outputs @@ -389,7 +389,7 @@ where } } - Ok(_pacs) + Ok(pacs) } /// Read process Primary Activity Commodities (PACs) from the specified model directory. From b814fefb281d56b2a7c8fb7d5824362156461f30 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Thu, 12 Dec 2024 16:45:09 +0000 Subject: [PATCH 05/11] Add valid flows to tests --- src/process.rs | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/process.rs b/src/process.rs index d7391a456..df8e5be4b 100644 --- a/src/process.rs +++ b/src/process.rs @@ -770,11 +770,51 @@ mod tests { costs: CommodityCostMap::new(), demand_by_region: HashMap::new(), }; - (Rc::clone(&commodity.id), commodity.into()) }) .collect(); - let flows: HashMap, Vec> = HashMap::new(); + let flows: HashMap, Vec> = [ + ( + "id1".into(), + vec![ + ProcessFlow { + process_id: "id1".into(), + commodity_id: "commodity1".into(), + flow: 1.0, + flow_type: FlowType::Fixed, + flow_cost: 1.0, + }, + ProcessFlow { + process_id: "id1".into(), + commodity_id: "commodity2".into(), + flow: 1.0, + flow_type: FlowType::Fixed, + flow_cost: 1.0, + }, + ], + ), + ( + "id2".into(), + vec![ + ProcessFlow { + process_id: "id2".into(), + commodity_id: "commodity1".into(), + flow: 1.0, + flow_type: FlowType::Fixed, + flow_cost: 1.0, + }, + ProcessFlow { + process_id: "id2".into(), + commodity_id: "commodity2".into(), + flow: 1.0, + flow_type: FlowType::Flexible, + flow_cost: 1.0, + }, + ], + ), + ] + .into_iter() + .collect(); // duplicate PAC let pac = ProcessPAC { From 17a8483f5487e0d8c9e80b2927ffcf208ca58f0b Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Fri, 13 Dec 2024 09:21:18 +0000 Subject: [PATCH 06/11] Make test code more concise --- src/process.rs | 62 ++++++++++++++++---------------------------------- 1 file changed, 19 insertions(+), 43 deletions(-) diff --git a/src/process.rs b/src/process.rs index df8e5be4b..b191194a9 100644 --- a/src/process.rs +++ b/src/process.rs @@ -344,7 +344,7 @@ where let process_id = process_ids.get_id(&pac.process_id)?; let commodity = commodities.get(pac.commodity_id.as_str()); - // Check that commodity is valid and not a duplicate + // Check that commodity is valid and PAC is not a duplicate match commodity { None => bail!("{} is not a valid commodity ID", &pac.commodity_id), Some(commodity) => { @@ -773,48 +773,24 @@ mod tests { (Rc::clone(&commodity.id), commodity.into()) }) .collect(); - let flows: HashMap, Vec> = [ - ( - "id1".into(), - vec![ - ProcessFlow { - process_id: "id1".into(), - commodity_id: "commodity1".into(), - flow: 1.0, - flow_type: FlowType::Fixed, - flow_cost: 1.0, - }, - ProcessFlow { - process_id: "id1".into(), - commodity_id: "commodity2".into(), - flow: 1.0, - flow_type: FlowType::Fixed, - flow_cost: 1.0, - }, - ], - ), - ( - "id2".into(), - vec![ - ProcessFlow { - process_id: "id2".into(), - commodity_id: "commodity1".into(), - flow: 1.0, - flow_type: FlowType::Fixed, - flow_cost: 1.0, - }, - ProcessFlow { - process_id: "id2".into(), - commodity_id: "commodity2".into(), - flow: 1.0, - flow_type: FlowType::Flexible, - flow_cost: 1.0, - }, - ], - ), - ] - .into_iter() - .collect(); + let flows: HashMap, Vec> = ["id1", "id2"] + .into_iter() + .map(|process_id| { + ( + process_id.into(), + ["commodity1", "commodity2"] + .into_iter() + .map(|commodity_id| ProcessFlow { + process_id: process_id.into(), + commodity_id: commodity_id.into(), + flow: 1.0, + flow_type: FlowType::Fixed, + flow_cost: 1.0, + }) + .collect(), + ) + }) + .collect(); // duplicate PAC let pac = ProcessPAC { From 096bc8bcb1120276212e5e688a3b47b1c598efa2 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Fri, 13 Dec 2024 09:42:39 +0000 Subject: [PATCH 07/11] Add test --- src/process.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/process.rs b/src/process.rs index b191194a9..46fe9d317 100644 --- a/src/process.rs +++ b/src/process.rs @@ -773,7 +773,7 @@ mod tests { (Rc::clone(&commodity.id), commodity.into()) }) .collect(); - let flows: HashMap, Vec> = ["id1", "id2"] + let mut flows: HashMap, Vec> = ["id1", "id2"] .into_iter() .map(|process_id| { ( @@ -816,8 +816,6 @@ mod tests { ) .is_err()); - // Invalid flows - // Valid let pacs = [ ProcessPAC { @@ -855,9 +853,28 @@ mod tests { .into_iter() .collect(); assert!( - read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities, &flows) - .unwrap() + read_process_pacs_from_iter( + pacs.clone().into_iter(), + &process_ids, + &commodities, + &flows + ) + .unwrap() == expected ); + + // Invalid flows + // Making commodity1 an input so the PACs are a mix of inputs and outputs + flows + .get_mut(&Rc::from("id1")) + .unwrap() + .iter_mut() + .find(|flow| flow.commodity_id == "commodity1") + .unwrap() + .flow = -1.0; + assert!( + read_process_pacs_from_iter(pacs.into_iter(), &process_ids, &commodities, &flows) + .is_err() + ); } } From 3cfcc1c02675530a10d63865fe52ca2c32e767c1 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Fri, 13 Dec 2024 09:45:59 +0000 Subject: [PATCH 08/11] Improve comment --- src/process.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/process.rs b/src/process.rs index 46fe9d317..b637e58cc 100644 --- a/src/process.rs +++ b/src/process.rs @@ -864,7 +864,7 @@ mod tests { ); // Invalid flows - // Making commodity1 an input so the PACs are a mix of inputs and outputs + // Making commodity1 an input so the PACs for process id1 are a mix of inputs and outputs flows .get_mut(&Rc::from("id1")) .unwrap() From 8dd650852dc7c8d4e10e85c83f4f4aba6b7bbd57 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Fri, 13 Dec 2024 10:11:40 +0000 Subject: [PATCH 09/11] Rename object for clarity --- src/process.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/process.rs b/src/process.rs index b637e58cc..d22207b2d 100644 --- a/src/process.rs +++ b/src/process.rs @@ -378,9 +378,9 @@ where None => { flow_sign = Some(current_flow_sign); } - Some(existing_flow_type) => { + Some(flow_sign) => { ensure!( - existing_flow_type == current_flow_sign, + current_flow_sign == flow_sign, "PACs for process {} are a mix of inputs and outputs", process_id ); @@ -388,7 +388,6 @@ where }; } } - Ok(pacs) } From 430860fd59d134082e38feca00b7c2b073001c37 Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Fri, 13 Dec 2024 12:26:19 +0000 Subject: [PATCH 10/11] Apply suggested changes from code review --- src/process.rs | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/src/process.rs b/src/process.rs index d22207b2d..0103c6291 100644 --- a/src/process.rs +++ b/src/process.rs @@ -4,7 +4,7 @@ use crate::input::*; use crate::region::*; use crate::time_slice::{TimeSliceInfo, TimeSliceSelection}; use ::log::warn; -use anyhow::{bail, ensure, Context, Result}; +use anyhow::{ensure, Context, Result}; use itertools::Itertools; use serde::{Deserialize, Deserializer}; use serde_string_enum::{DeserializeLabeledStringEnum, SerializeLabeledStringEnum}; @@ -54,7 +54,9 @@ pub struct ProcessAvailability { } define_process_id_getter! {ProcessAvailability} -#[derive(PartialEq, Default, Debug, SerializeLabeledStringEnum, DeserializeLabeledStringEnum)] +#[derive( + PartialEq, Default, Debug, Clone, SerializeLabeledStringEnum, DeserializeLabeledStringEnum, +)] pub enum FlowType { #[default] #[string = "fixed"] @@ -65,7 +67,7 @@ pub enum FlowType { Flexible, } -#[derive(PartialEq, Debug, Deserialize)] +#[derive(PartialEq, Debug, Deserialize, Clone)] pub struct ProcessFlow { /// A unique identifier for the process (typically uses a structured naming convention). pub process_id: String, @@ -342,21 +344,19 @@ where let pacs = iter .map(|pac| { let process_id = process_ids.get_id(&pac.process_id)?; - let commodity = commodities.get(pac.commodity_id.as_str()); + let commodity = commodities + .get(pac.commodity_id.as_str()) + .with_context(|| format!("{} is not a valid commodity ID", &pac.commodity_id))?; // Check that commodity is valid and PAC is not a duplicate - match commodity { - None => bail!("{} is not a valid commodity ID", &pac.commodity_id), - Some(commodity) => { - ensure!(existing_pacs.insert(pac), "Duplicate PACs found"); - Ok((process_id, Rc::clone(commodity))) - } - } + ensure!(existing_pacs.insert(pac), "Duplicate PACs found"); + Ok((process_id, Rc::clone(commodity))) }) .process_results(|iter| iter.into_group_map())?; // Check that PACs for each process are either all inputs or all outputs for (process_id, pacs) in pacs.iter() { + // Get the flows for the process (unwrap is safe as every process has associated flows) let flows = flows.get(process_id).unwrap(); let mut flow_sign: Option = None; // False for inputs, true for outputs @@ -374,18 +374,14 @@ where // Check that flow sign is consistent let current_flow_sign = flow.flow > 0.0; - match flow_sign { - None => { - flow_sign = Some(current_flow_sign); - } - Some(flow_sign) => { - ensure!( - current_flow_sign == flow_sign, - "PACs for process {} are a mix of inputs and outputs", - process_id - ); - } - }; + if let Some(flow_sign) = flow_sign { + ensure!( + current_flow_sign == flow_sign, + "PACs for process {} are a mix of inputs and outputs", + process_id + ); + } + flow_sign = Some(current_flow_sign); } } Ok(pacs) @@ -772,7 +768,7 @@ mod tests { (Rc::clone(&commodity.id), commodity.into()) }) .collect(); - let mut flows: HashMap, Vec> = ["id1", "id2"] + let flows: HashMap, Vec> = ["id1", "id2"] .into_iter() .map(|process_id| { ( @@ -864,6 +860,7 @@ mod tests { // Invalid flows // Making commodity1 an input so the PACs for process id1 are a mix of inputs and outputs + let mut flows = flows.clone(); flows .get_mut(&Rc::from("id1")) .unwrap() From a9623a93b684a4c004f4111775a5e5110eaab96b Mon Sep 17 00:00:00 2001 From: Tom Bland Date: Mon, 16 Dec 2024 13:27:37 +0000 Subject: [PATCH 11/11] Break validation step into separate function --- src/process.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/process.rs b/src/process.rs index 0103c6291..da8952e7d 100644 --- a/src/process.rs +++ b/src/process.rs @@ -355,6 +355,25 @@ where .process_results(|iter| iter.into_group_map())?; // Check that PACs for each process are either all inputs or all outputs + validate_pac_flows(&pacs, flows)?; + + // Return result + Ok(pacs) +} + +/// Validate that the PACs for each process are either all inputs or all outputs. +/// +/// # Arguments +/// +/// * `pacs` - A map of process IDs to PAC commodities +/// * `flows` - A map of process IDs to process flows +/// +/// # Returns +/// An `Ok(())` if the check is successful, or an error. +fn validate_pac_flows( + pacs: &HashMap, Vec>>, + flows: &HashMap, Vec>, +) -> Result<()> { for (process_id, pacs) in pacs.iter() { // Get the flows for the process (unwrap is safe as every process has associated flows) let flows = flows.get(process_id).unwrap(); @@ -384,7 +403,7 @@ where flow_sign = Some(current_flow_sign); } } - Ok(pacs) + Ok(()) } /// Read process Primary Activity Commodities (PACs) from the specified model directory.