Skip to content

Comments

Add validation for SVD commodities#381

Merged
TinyMarsh merged 4 commits intomainfrom
validate-commodity-process
Mar 4, 2025
Merged

Add validation for SVD commodities#381
TinyMarsh merged 4 commits intomainfrom
validate-commodity-process

Conversation

@TinyMarsh
Copy link
Collaborator

@TinyMarsh TinyMarsh commented Feb 5, 2025

Description

Adds validation for SVD commodities by checking that for every commodity of "SVD" type, there is at least one process that can produce the commodity. Also extended this to check for every region and year and time slice, where SVD is > 0, that there is a process defined that can meet demand.

Fixes #168

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

@TinyMarsh TinyMarsh marked this pull request as ready for review February 5, 2025 10:17
@codecov
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 95.31250% with 6 lines in your changes missing coverage. Please review.

Project coverage is 95.60%. Comparing base (54e9a7f) to head (2bac877).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/input/process.rs 96.03% 2 Missing and 3 partials ⚠️
src/commodity.rs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
+ Coverage   95.58%   95.60%   +0.02%     
==========================================
  Files          31       31              
  Lines        4121     4235     +114     
  Branches     4121     4235     +114     
==========================================
+ Hits         3939     4049     +110     
- Misses         90       92       +2     
- Partials       92       94       +2     

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

Copy link
Collaborator

@tsmbland tsmbland left a comment

Choose a reason for hiding this comment

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

Looks good, but I'll let @alexdewar give the final go-ahead.

The only comment I have is that the process must also have availability > 0, otherwise even with flow > 0 it cannot produce any output. So really, I think we need to check that there's at least one process with flow * availability > 0 in every timeslice/year/region with demand

Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

Looks good!

I hadn't thought of this, but @tsmbland's right about the availability thing: it does need an extra check that availability > 0 too. Otherwise all good 😄

.into_iter()
.collect();

let regions: HashMap<Rc<str>, RegionSelection> = vec![
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let regions: HashMap<Rc<str>, RegionSelection> = vec![
let regions: HashMap<Rc<str>, RegionSelection> = [

@TinyMarsh
Copy link
Collaborator Author

Apologies for taking a while to get back to this.

To be honest I don't really understand why process availabilities are ranges, but I have included a check that the range is above zero in any case. I hope that's correct.

I had to factor out some of the arguments because Clippy wasn't happy, hence the ValidationParams struct. If this is horrible then let me know and I'll change it.

@TinyMarsh
Copy link
Collaborator Author

@alexdewar Latest commit just includes changes we went through together.

@TinyMarsh TinyMarsh requested a review from alexdewar March 3, 2025 16:05
Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry the last part ended up being a faff....

@TinyMarsh TinyMarsh merged commit 3e871f6 into main Mar 4, 2025
7 checks passed
@TinyMarsh TinyMarsh deleted the validate-commodity-process branch March 4, 2025 09:22
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.

For every commodity of "SVD" type, there must be at least one process that can produce the commodity

3 participants