-
Notifications
You must be signed in to change notification settings - Fork 2
Validate flows for non-primary outputs/inputs #962
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
27702fe
778d626
36de838
deb6d57
03f3cfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| use super::super::{input_err_msg, read_csv}; | ||
| use crate::commodity::{CommodityID, CommodityMap}; | ||
| use crate::process::{FlowType, ProcessFlow, ProcessFlowsMap, ProcessID, ProcessMap}; | ||
| use crate::region::parse_region_str; | ||
| use crate::region::{RegionID, parse_region_str}; | ||
| use crate::units::{FlowPerActivity, MoneyPerFlow}; | ||
| use crate::year::parse_year_str; | ||
| use anyhow::{Context, Result, ensure}; | ||
|
|
@@ -46,7 +46,7 @@ impl ProcessFlowRaw { | |
| // Check that flow cost is non-negative | ||
| if let Some(cost) = self.cost { | ||
| ensure!( | ||
| (0.0..f64::INFINITY).contains(&cost.value()), | ||
| (cost.value() >= 0.0), | ||
| "Invalid value for flow cost ({cost}). Must be >=0." | ||
| ); | ||
| } | ||
|
|
@@ -132,6 +132,7 @@ where | |
| } | ||
|
|
||
| validate_flows_and_update_primary_output(processes, &flows_map)?; | ||
| validate_secondary_flows(processes, &flows_map)?; | ||
|
|
||
| Ok(flows_map) | ||
| } | ||
|
|
@@ -229,6 +230,53 @@ fn check_flows_primary_output( | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Checks that non-primary io are defined for all years (within a region) and that | ||
| /// they are only inputs or only outputs in all years. | ||
| fn validate_secondary_flows( | ||
| processes: &mut ProcessMap, | ||
| flows_map: &HashMap<ProcessID, ProcessFlowsMap>, | ||
| ) -> Result<()> { | ||
| for (process_id, process) in processes.iter() { | ||
| // Get the flows for this process - there should be no error, as was checked already | ||
| let map = flows_map | ||
| .get(process_id) | ||
| .with_context(|| format!("Missing flows map for process {process_id}"))?; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this function and Not particularly important though.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it, but I did not want to intervene the existing functionality too much. Also, being used just during loading of data, didn't consider performance to be an issue.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking more in terms of tidiness than performance, but I agree that it's ok as it is! |
||
|
|
||
| // Get the non-primary io flows for all years, if any, arranged by (commodity, region) | ||
| let iter = iproduct!(process.years.iter(), process.regions.iter()); | ||
| let mut flows: HashMap<(CommodityID, RegionID), Vec<bool>> = HashMap::new(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this code could be simpler if you just included the year in the key, i.e. made this a
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not need the year for anything, so why storing that information at all? If I do that, it will also be harder to check if all the values for a specific commodity and region combination are of the same sign and complete.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm not quite sure what I meant here... it is fine the way it is. |
||
| for (&year, region_id) in iter { | ||
| let flow = map[&(region_id.clone(), year)] | ||
| .iter() | ||
| .filter_map(|(commodity_id, flow)| { | ||
| (Some(commodity_id) != process.primary_output.as_ref()) | ||
| .then_some(((commodity_id.clone(), region_id.clone()), flow.is_input())) | ||
| }); | ||
|
|
||
| for (key, value) in flow { | ||
| flows.entry(key).or_default().push(value); | ||
| } | ||
| } | ||
|
|
||
| // Finally we check that the flows for a given commodity and region are defined for all | ||
| // years and that they are all inputs or all outputs | ||
| for ((commodity_id, region_id), value) in &flows { | ||
| ensure!( | ||
| value.len() == process.years.len(), | ||
| "Flow of commodity {commodity_id} in region {region_id} for process {process_id} \ | ||
| does not cover all years" | ||
| ); | ||
| ensure!( | ||
| value.iter().all(|&x| x == value[0]), | ||
| "Flow of commodity {commodity_id} in region {region_id} for process {process_id} \ | ||
| behaves as input or output in different years." | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was weird the way it was written before! I guess the previous check also verified that it was finite, but that's not a worry since we do that check when we deserialise now anyway