Skip to content

Add region_id to process_parameters table#498

Merged
tsmbland merged 10 commits intomainfrom
process_parameters_region
Apr 29, 2025
Merged

Add region_id to process_parameters table#498
tsmbland merged 10 commits intomainfrom
process_parameters_region

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented Apr 24, 2025

Description

Continues on from #475, adding a region dimension

Fixes #493

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

@tsmbland tsmbland changed the title Add region_id to process_parameters table Add region_id to process_parameters table Apr 24, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2025

Codecov Report

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

Project coverage is 82.85%. Comparing base (a8097a3) to head (f075cfd).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/input/process/parameter.rs 68.00% 3 Missing and 5 partials ⚠️
src/input.rs 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #498       +/-   ##
===========================================
- Coverage   94.32%   82.85%   -11.48%     
===========================================
  Files          36       36               
  Lines        4673     4689       +16     
  Branches     4673     4689       +16     
===========================================
- Hits         4408     3885      -523     
- Misses        133      706      +573     
+ Partials      132       98       -34     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsmbland tsmbland marked this pull request as ready for review April 24, 2025 09:50
@tsmbland tsmbland requested a review from alexdewar April 24, 2025 09:50
@tsmbland
Copy link
Copy Markdown
Collaborator Author

This will have to change now that YearSelection is no more. While we're at it, I think now is the time to get rid of RegionSelection as well

@tsmbland tsmbland marked this pull request as draft April 28, 2025 11:00
Base automatically changed from annual_field_alternative to main April 28, 2025 18:44
@tsmbland tsmbland changed the base branch from main to region_selection April 29, 2025 12:59
@tsmbland tsmbland marked this pull request as ready for review April 29, 2025 13:05
@tsmbland
Copy link
Copy Markdown
Collaborator Author

Ok ready now!

Copy link
Copy Markdown
Member

@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.

Two small suggestions, but otherwise all good!

Comment thread src/input/process/parameter.rs Outdated
// Check parameters cover all years and regions of the process
for (id, parameters) in map.iter() {
let process = processes.get(id).unwrap();
let year_range = process.years.clone();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let year_range = process.years.clone();
let year_range = &process.years;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

How are so good at spotting these sorts of things?! And why isn't this picked up by clippy?

Comment thread src/input/process/parameter.rs Outdated
Comment on lines +168 to +170
if !parameters.contains_key(&(region.clone(), *year)) {
missing_keys.push((region, *year));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should work:

Suggested change
if !parameters.contains_key(&(region.clone(), *year)) {
missing_keys.push((region, *year));
}
let key = (region, *year);
if !parameters.contains_key(&key) {
missing_keys.push(key);
}

Base automatically changed from region_selection to main April 29, 2025 15:34
@tsmbland tsmbland enabled auto-merge April 29, 2025 15:39
@tsmbland tsmbland merged commit 543d3ea into main Apr 29, 2025
7 checks passed
@tsmbland tsmbland deleted the process_parameters_region branch April 29, 2025 15:40
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.

Add "year" and "region" fields to process_parameters.csv and associated data structures

2 participants