Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #614 +/- ##
==========================================
+ Coverage 85.39% 85.58% +0.18%
==========================================
Files 38 38
Lines 3410 3357 -53
Branches 3410 3357 -53
==========================================
- Hits 2912 2873 -39
+ Misses 305 295 -10
+ Partials 193 189 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
The PR removes all references to Primary Activity Commodities (PACs) across code, tests, schemas, and docs to align with the new model. Key changes include:
- Dropped
is_pacfields, PAC-specific methods, and related validations. - Updated documentation comments, input schemas, and glossary to eliminate PAC terminology.
- Simplified flow parsing and validation logic by removing PAC-centric sorting and checks.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/simulation/optimisation/constraints.rs | Update capacity constraint docs to remove PAC terminology |
| src/process.rs | Remove is_pac fields, iter_pacs, and update docs |
| src/input/process/flow.rs | Remove PAC import, fields, sorting, validation, and tests |
| src/input/process/availability.rs | Update availability doc comments to remove PAC references |
| src/input/process.rs | Remove PAC flag in test fixtures |
| src/asset.rs | Update asset methods and docs to remove PAC mention |
| schemas/input/process_parameters.yaml | Remove PAC-specific titles from schema |
| schemas/input/process_flows.yaml | Remove PAC-related schema fields |
| docs/glossary.md | Eliminate PAC glossary entries |
Comments suppressed due to low confidence (3)
src/input/process/flow.rs:151
- The error message uses braces
{}with variable names but they won't be interpolated. Use a formatted string with positional{}placeholders and pass the variables, e.g.,ensure!(cond, "Missing entry for process {} in {}/{}", process_id, region, year);.
ensure!(
map.contains_key(&(region.clone(), *year)),
"Missing entry for process {process_id} in {region}/{year}"
);
src/input/process/flow.rs:144
- [nitpick] The parameter
mapis ambiguous; consider renaming it toflows_maporprocess_flowsfor clarity.
fn validate_process_flows_map(process: &Process, map: &ProcessFlowsMap) -> Result<()> {
src/simulation/optimisation/constraints.rs:106
- [nitpick] The phrase "for assets" is unclear; consider changing it to "for each asset" or "of the asset" to clarify the comment.
/// For every asset at every time slice, the sum of the commodity flows for assets must not exceed
tsmbland
left a comment
There was a problem hiding this comment.
I'm going to approve, but I suspect a few things are not quite right.
In particular, now that PACs are gone, I've think we've fundamentally changed the definition of "activity". Whereas before, this was measured in absolute units of the PAC per year (e.g, PJ/year), I think now it represents quantities of the flow coefficients. So if flow coefficients are e.g. PJ/year, then activity is actually unitless.
I'm starting to think more and more that a formal units system, like I proposed in #481, is the only way to be truly robust about this. Otherwise I could well see us introducing silly errors that would be difficult/impossible to spot.
| pub years: Vec<u32>, | ||
| /// Limits on PAC energy consumption/production for each time slice (as a fraction of maximum) | ||
| /// Limits on energy consumption/production for each time slice (as a fraction of maximum) | ||
| pub energy_limits: ProcessEnergyLimitsMap, |
There was a problem hiding this comment.
I think now that PACs are gone, this now an activity limit rather than an energy limit (i.e. needs to be multiplied by flow coefficients to get energy limits for each commodity)
There was a problem hiding this comment.
True. I'll update this.
Good point. I'm sure there are mistakes dotted around. Hopefully we'll catch some of them as we go along, but it's probably worth skimming the docs again before we make a release.
I think this is v good idea. Hopefully some of this stuff will be clearer in our own minds by the end of the engagement, but even then I can see it would be easy to make a mistake. And it'll be even easier for someone new to the project/the MUSE model. The nice thing about using a language like Rust is that the unit types would be a zero-cost abstraction (i.e. the program would run just as fast as if you were just using raw I did think that we could even go one step further and add runtime checks for commodity flows to check that we aren't mixing up flows for different commodities (e.g. when adding them). This wouldn't be zero-cost though (it would probably hammer performance), so we'd probably want to make it a compile-time option (e.g. just do it for debug builds). |
Co-authored-by: Tom Bland <t.bland@imperial.ac.uk>
Seems I overlooked this.
Description
The concept of PACs is going away in the new model, so we don't need to worry about them anymore. Huzzah!
Remove them from the code and documentation. I've done my best to update doc comments etc. so that they still make sense, but mistakes might have crept in.
Closes #596.
Type of change
Key checklist
$ cargo test$ cargo docFurther checks