Skip to content

Comments

Add some more validation checks for process flows#323

Merged
alexdewar merged 13 commits intomainfrom
more-process-flow-checks
Jan 17, 2025
Merged

Add some more validation checks for process flows#323
alexdewar merged 13 commits intomainfrom
more-process-flow-checks

Conversation

@alexdewar
Copy link
Collaborator

@alexdewar alexdewar commented Jan 10, 2025

Description

I've added some extra validation steps related to process flows, namely:

  • Check that process flows are valid numbers
  • Check that flow costs are valid numbers
  • Check that every process has at least one associated process flow (i.e. it produces or consumes something)
  • Disallow commodity flexible flows for now (we will only support fixed ones for now)

I also added a check that every process has at least one PAC.

Closes #277. Closes #278. Closes #279.

Type of change

  • Bug fix (non-breaking change to fix an issue)
  • New feature (non-breaking change to add functionality)
  • Refactoring (non-breaking, non-functional change to improve maintainability)
  • Optimization (non-breaking change to speed up the code)
  • Breaking change (whatever its nature)
  • Documentation (improve or add documentation)

Key checklist

  • All tests pass: $ cargo test
  • The documentation builds and looks OK: $ cargo doc

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 95.65217% with 9 lines in your changes missing coverage. Please review.

Project coverage is 95.56%. Comparing base (1d8072a) to head (d4fca15).
Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
src/input/process.rs 95.80% 0 Missing and 6 partials ⚠️
src/input/process/availability.rs 77.77% 0 Missing and 2 partials ⚠️
src/input/process/flow.rs 98.18% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   95.29%   95.56%   +0.27%     
==========================================
  Files          26       26              
  Lines        2613     2751     +138     
  Branches     2613     2751     +138     
==========================================
+ Hits         2490     2629     +139     
  Misses         47       47              
+ Partials       76       75       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexdewar alexdewar force-pushed the more-process-flow-checks branch from 3cde296 to 2244093 Compare January 13, 2025 12:06
@alexdewar alexdewar marked this pull request as ready for review January 13, 2025 12:08
Copy link
Collaborator

@AdrianDAlessandro AdrianDAlessandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

.with_context(|| format!("No commodity flows defined for process {id}"))?;
let pacs = pacs
.remove(&id)
.with_context(|| format!("No PACs defined for process {id}"))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add tests for these two cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably. I was being a bit lazy here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to write tests, I split out the map creation stuff into its own function. I also figured that this function is the right place to check for other missing process data (e.g. flows, availabilities). I had been checking for this by just checking that the data structures had the same length as the process_ids one, but by doing the check when we actually need the data for a given process, we can tell the user which process is missing this data, so I think that's an improvement.

I: Iterator<Item = ProcessFlowRaw>,
{
iter.map(|flow_raw| -> Result<ProcessFlow> {
iter.map(|flow| -> Result<ProcessFlow> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the name change here? As far as I can tell, it is still a ProcessFlowRaw that is being iterated over.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was more readable that way... Maybe not though

// Check that flow is not zero, infinity, etc.
ensure!(
flow.flow.is_normal(),
"Invalid value for flow: {}",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain the restriction on the value in this message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a separate check for zero values instead, because I think that's the one that's likely to trip users up. If they put nan as a value for flow, we probably don't need to explain why that's not allowed!

@alexdewar alexdewar merged commit 25bd24c into main Jan 17, 2025
7 checks passed
@alexdewar alexdewar deleted the more-process-flow-checks branch January 17, 2025 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure every process has associated flows Check flow cost is a valid number Check process flow is valid number

2 participants