From e11cad9840129d382f70aea3476ec1c9402e0076 Mon Sep 17 00:00:00 2001 From: Diego Alonso Alvarez Date: Thu, 7 Nov 2024 07:03:00 +0000 Subject: [PATCH 1/5] :sparkles: Add validation for proceses. --- src/process.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/process.rs b/src/process.rs index 31d1e8d2..9adec52e 100644 --- a/src/process.rs +++ b/src/process.rs @@ -117,6 +117,8 @@ impl ProcessParameterRaw { ))?; } + self.validate()?; + Ok(ProcessParameter { process_id: self.process_id, years: start_year..=end_year, @@ -130,6 +132,30 @@ impl ProcessParameterRaw { } } +impl ProcessParameterRaw { + fn validate(&self) -> Result<(), Box> { + if self.lifetime == 0 { + Err(format!( + "Error in parameter for process {}: Lifetime must be positive", + self.process_id + ))?; + } + if self.discount_rate.is_some() && self.discount_rate.unwrap() < 0.0 { + Err(format!( + "Error in parameter for process {}: Discount rate must be greater than 0", + self.process_id + ))?; + } + if self.cap2act.is_some() && self.cap2act.unwrap() < 0.0 { + Err(format!( + "Error in parameter for process {}: Cap2act must be greater than 0", + self.process_id + ))?; + } + Ok(()) + } +} + #[derive(PartialEq, Debug, Deserialize)] pub struct ProcessParameter { pub process_id: String, From b98dd39957fb29d503b4c1412e53dbb82cb9b715 Mon Sep 17 00:00:00 2001 From: Diego Alonso Alvarez Date: Thu, 7 Nov 2024 08:51:23 +0000 Subject: [PATCH 2/5] :sparkles: Add warning and tests. --- src/process.rs | 67 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/src/process.rs b/src/process.rs index 9adec52e..bd17c64a 100644 --- a/src/process.rs +++ b/src/process.rs @@ -1,6 +1,7 @@ use crate::input::*; use crate::region::*; use crate::time_slice::{TimeSliceInfo, TimeSliceSelection}; +use ::log::warn; use serde::{Deserialize, Deserializer}; use serde_string_enum::{DeserializeLabeledStringEnum, SerializeLabeledStringEnum}; use std::collections::{HashMap, HashSet}; @@ -134,24 +135,30 @@ impl ProcessParameterRaw { impl ProcessParameterRaw { fn validate(&self) -> Result<(), Box> { - if self.lifetime == 0 { + if self.lifetime <= 0 { Err(format!( - "Error in parameter for process {}: Lifetime must be positive", + "Error in parameter for process {}: Lifetime must be greater than 0", self.process_id ))?; } if self.discount_rate.is_some() && self.discount_rate.unwrap() < 0.0 { Err(format!( - "Error in parameter for process {}: Discount rate must be greater than 0", + "Error in parameter for process {}: Discount rate must be positive", self.process_id ))?; } if self.cap2act.is_some() && self.cap2act.unwrap() < 0.0 { Err(format!( - "Error in parameter for process {}: Cap2act must be greater than 0", + "Error in parameter for process {}: Cap2act must be positive", self.process_id ))?; } + if self.discount_rate.is_some() && self.discount_rate.unwrap() > 1.0 { + warn!( + "Warning in parameter for process {}: Discount rate is greater than 1", + self.process_id + ); + } Ok(()) } } @@ -351,6 +358,7 @@ mod tests { fn create_param_raw( start_year: Option, end_year: Option, + lifetime: u32, discount_rate: Option, cap2act: Option, ) -> ProcessParameterRaw { @@ -361,7 +369,7 @@ mod tests { capital_cost: 0.0, fixed_operating_cost: 0.0, variable_operating_cost: 0.0, - lifetime: 1, + lifetime, discount_rate, cap2act, } @@ -389,28 +397,28 @@ mod tests { let year_range = 2000..=2100; // No missing values - let raw = create_param_raw(Some(2010), Some(2020), Some(1.0), Some(0.0)); + let raw = create_param_raw(Some(2010), Some(2020), 1, Some(1.0), Some(0.0)); assert_eq!( raw.into_parameter(&year_range).unwrap(), create_param(2010..=2020, 1.0, 0.0) ); // Missing years - let raw = create_param_raw(None, None, Some(1.0), Some(0.0)); + let raw = create_param_raw(None, None, 1, Some(1.0), Some(0.0)); assert_eq!( raw.into_parameter(&year_range).unwrap(), create_param(2000..=2100, 1.0, 0.0) ); // Missing discount_rate - let raw = create_param_raw(Some(2010), Some(2020), None, Some(0.0)); + let raw = create_param_raw(Some(2010), Some(2020), 1, None, Some(0.0)); assert_eq!( raw.into_parameter(&year_range).unwrap(), create_param(2010..=2020, 0.0, 0.0) ); // Missing cap2act - let raw = create_param_raw(Some(2010), Some(2020), Some(1.0), None); + let raw = create_param_raw(Some(2010), Some(2020), 1, Some(1.0), None); assert_eq!( raw.into_parameter(&year_range).unwrap(), create_param(2010..=2020, 1.0, 1.0) @@ -423,21 +431,21 @@ mod tests { // Normal case assert!( - create_param_raw(Some(2000), Some(2100), Some(1.0), Some(0.0)) + create_param_raw(Some(2000), Some(2100), 1, Some(1.0), Some(0.0)) .into_parameter(&year_range) .is_ok() ); // start_year out of range - this is permitted assert!( - create_param_raw(Some(1999), Some(2100), Some(1.0), Some(0.0)) + create_param_raw(Some(1999), Some(2100), 1, Some(1.0), Some(0.0)) .into_parameter(&year_range) .is_ok() ); // end_year out of range - this is permitted assert!( - create_param_raw(Some(2000), Some(2101), Some(1.0), Some(0.0)) + create_param_raw(Some(2000), Some(2101), 1, Some(1.0), Some(0.0)) .into_parameter(&year_range) .is_ok() ); @@ -450,12 +458,45 @@ mod tests { // start_year after end_year assert!( - create_param_raw(Some(2001), Some(2000), Some(1.0), Some(0.0)) + create_param_raw(Some(2001), Some(2000), 1, Some(1.0), Some(0.0)) .into_parameter(&year_range) .is_ok() ); } + #[test] + #[should_panic] + fn test_param_raw_validate_bad_lifetime() { + // lifetime = 0 + assert!( + create_param_raw(Some(2000), Some(2100), 0, Some(1.0), Some(0.0)) + .validate() + .is_ok() + ); + } + + #[test] + #[should_panic] + fn test_param_raw_validate_bad_discount_rate() { + // discount rate = -1 + assert!( + create_param_raw(Some(2000), Some(2100), 0, Some(-1.0), Some(0.0)) + .validate() + .is_ok() + ); + } + + #[test] + #[should_panic] + fn test_param_raw_validate_bad_capt2act() { + // capt2act = -1 + assert!( + create_param_raw(Some(2000), Some(2100), 0, Some(1.0), Some(-1.0)) + .validate() + .is_ok() + ); + } + #[test] fn test_read_process_parameters_from_iter_good() { let year_range = 2000..=2100; From c926e7d6b80c3fc61591f9f6106121bf9ecaa782 Mon Sep 17 00:00:00 2001 From: Diego Alonso Alvarez Date: Thu, 7 Nov 2024 09:01:01 +0000 Subject: [PATCH 3/5] :memo: Add docstring. --- src/process.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/process.rs b/src/process.rs index bd17c64a..cccdb0fe 100644 --- a/src/process.rs +++ b/src/process.rs @@ -134,8 +134,25 @@ impl ProcessParameterRaw { } impl ProcessParameterRaw { + /// Validates the `ProcessParameterRaw` instance. + /// + /// # Errors + /// + /// Returns an error if: + /// - `lifetime` is 0. + /// - `discount_rate` is present and less than 0.0. + /// - `cap2act` is present and less than 0.0. + /// + /// # Warnings + /// + /// Logs a warning if: + /// - `discount_rate` is present and greater than 1.0. + /// + /// # Returns + /// + /// Returns `Ok(())` if all validations pass. fn validate(&self) -> Result<(), Box> { - if self.lifetime <= 0 { + if self.lifetime == 0 { Err(format!( "Error in parameter for process {}: Lifetime must be greater than 0", self.process_id From 186a5419f782aec97bceaeef6a5ed0f5ea1cd11f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diego=20Alonso=20=C3=81lvarez?= <6095790+dalonsoa@users.noreply.github.com> Date: Thu, 7 Nov 2024 11:14:01 +0000 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Alex Dewar --- src/process.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/process.rs b/src/process.rs index cccdb0fe..f8bc5a79 100644 --- a/src/process.rs +++ b/src/process.rs @@ -158,11 +158,13 @@ impl ProcessParameterRaw { self.process_id ))?; } - if self.discount_rate.is_some() && self.discount_rate.unwrap() < 0.0 { - Err(format!( - "Error in parameter for process {}: Discount rate must be positive", - self.process_id - ))?; + if let Some(dr) = self.discount_rate { + if dr < 0.0 { + Err(format!( + "Error in parameter for process {}: Discount rate must be positive", + self.process_id + ))?; + } } if self.cap2act.is_some() && self.cap2act.unwrap() < 0.0 { Err(format!( @@ -482,13 +484,12 @@ mod tests { } #[test] - #[should_panic] fn test_param_raw_validate_bad_lifetime() { // lifetime = 0 assert!( create_param_raw(Some(2000), Some(2100), 0, Some(1.0), Some(0.0)) .validate() - .is_ok() + .is_err() ); } From 4cf26285c72d6bebc932127c0b1dfe07f73779ad Mon Sep 17 00:00:00 2001 From: Diego Alonso Alvarez Date: Thu, 7 Nov 2024 11:20:51 +0000 Subject: [PATCH 5/5] :recycle: Add reviewer's comments. --- src/process.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/process.rs b/src/process.rs index f8bc5a79..a52176f6 100644 --- a/src/process.rs +++ b/src/process.rs @@ -165,18 +165,20 @@ impl ProcessParameterRaw { self.process_id ))?; } + if dr > 1.0 { + warn!( + "Warning in parameter for process {}: Discount rate is greater than 1", + self.process_id + ); + } } - if self.cap2act.is_some() && self.cap2act.unwrap() < 0.0 { - Err(format!( - "Error in parameter for process {}: Cap2act must be positive", - self.process_id - ))?; - } - if self.discount_rate.is_some() && self.discount_rate.unwrap() > 1.0 { - warn!( - "Warning in parameter for process {}: Discount rate is greater than 1", - self.process_id - ); + if let Some(c2a) = self.cap2act { + if c2a < 0.0 { + Err(format!( + "Error in parameter for process {}: Cap2act must be positive", + self.process_id + ))?; + } } Ok(()) } @@ -494,24 +496,22 @@ mod tests { } #[test] - #[should_panic] fn test_param_raw_validate_bad_discount_rate() { // discount rate = -1 assert!( create_param_raw(Some(2000), Some(2100), 0, Some(-1.0), Some(0.0)) .validate() - .is_ok() + .is_err() ); } #[test] - #[should_panic] fn test_param_raw_validate_bad_capt2act() { // capt2act = -1 assert!( create_param_raw(Some(2000), Some(2100), 0, Some(1.0), Some(-1.0)) .validate() - .is_ok() + .is_err() ); }