Skip to content

Add is_primary_output column to process_flows.csv#657

Merged
alexdewar merged 1 commit intomainfrom
primary-output
Jun 24, 2025
Merged

Add is_primary_output column to process_flows.csv#657
alexdewar merged 1 commit intomainfrom
primary-output

Conversation

@alexdewar
Copy link
Copy Markdown
Member

Description

Although we don't need PACs any more, we do have to identify one output flow as the "primary" for each process for the investment appraisal step.

I've added an is_primary_output column to process_flows.csv, but it's an optional value. If there are no output flows, then no primary output is needed. If there's only one, we can infer that it's the primary. If there is more than one, the user can get away with just marking one as true.

I haven't added a helper to get the primary output yet, because I wasn't sure exactly what would be the most useful form (e.g. do we want to return a ProcessFlow or just a Commodity or something). It might make sense to sort the process flows so that the primary output is always the first element, which would make looking it up faster, but I haven't done this yet.

Closes #630.

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

@alexdewar alexdewar requested a review from Copilot June 24, 2025 09:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds an optional is_primary_output column to the process_flows.csv file to support identifying a primary output for each process. Key changes include:

  • Adding an is_primary_output field to the ProcessFlow struct and updating its related tests.
  • Enhancing CSV reading logic to parse the new column and infer/validate primary outputs.
  • Updating sample CSV files to include the new column.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/process.rs Added is_primary_output field and updated tests accordingly
src/input/process/flow.rs Updated CSV parser to include is_primary_output and added new primary output validation logic
src/input/process.rs Adjusted tests to include is_primary_output in process flows
examples/simple_mc/process_flows.csv Added is_primary_output column to CSV sample
examples/simple/process_flows.csv Added is_primary_output column to CSV sample

Comment thread src/input/process/flow.rs
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 24, 2025

Codecov Report

Attention: Patch coverage is 88.34951% with 12 lines in your changes missing coverage. Please review.

Project coverage is 88.57%. Comparing base (6ab9ed2) to head (213336f).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/input/process/flow.rs 88.34% 9 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #657      +/-   ##
==========================================
+ Coverage   88.47%   88.57%   +0.09%     
==========================================
  Files          39       39              
  Lines        3558     3623      +65     
  Branches     3558     3623      +65     
==========================================
+ Hits         3148     3209      +61     
- Misses        219      221       +2     
- Partials      191      193       +2     

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

@alexdewar alexdewar requested a review from tsmbland June 24, 2025 09:47
Copy link
Copy Markdown
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.

I think it would have been a lot cleaner to have this as a column in the processes.csv table (since there's one per process and it should never vary by region and year), but I guess this is ok too

@alexdewar
Copy link
Copy Markdown
Member Author

I think it would have been a lot cleaner to have this as a column in the processes.csv table (since there's one per process and it should never vary by region and year), but I guess this is ok too

Oh really? That was my initial idea, but then I remembered we do vary flows by region and year, so thought this was the best way to do it.

It would be good to know what the "rules" are for varying flows, e.g. currently we allow users to vary what commodities are input/output by region and year, not just the magnitude of flows, which seems a bit odd.

@alexdewar alexdewar merged commit 8c08999 into main Jun 24, 2025
7 checks passed
@alexdewar alexdewar deleted the primary-output branch June 24, 2025 13:31
@tsmbland
Copy link
Copy Markdown
Collaborator

I think it would have been a lot cleaner to have this as a column in the processes.csv table (since there's one per process and it should never vary by region and year), but I guess this is ok too

Oh really? That was my initial idea, but then I remembered we do vary flows by region and year, so thought this was the best way to do it.

It would be good to know what the "rules" are for varying flows, e.g. currently we allow users to vary what commodities are input/output by region and year, not just the magnitude of flows, which seems a bit odd.

Yeah I think the primary output is core to the definition of the process so wouldn't make sense to vary this by region and year - and I think it's confusing to make this an option

I guess it doesn't make sense to vary the input/output commodities either, but I guess someone could conceivably want too set one of these flows to zero in a particular year, which we don't permit, so the only option in this case would be to remove it

@alexdewar
Copy link
Copy Markdown
Member Author

alexdewar commented Jun 24, 2025

Yeah I think the primary output is core to the definition of the process so wouldn't make sense to vary this by region and year - and I think it's confusing to make this an option

Maybe let's revisit this later then. I'll open an issue.

I guess it doesn't make sense to vary the input/output commodities either, but I guess someone could conceivably want too set one of these flows to zero in a particular year, which we don't permit, so the only option in this case would be to remove it

I wonder if it might make for a cleaner interface if we forced users to provide the same flows for every year and region and just made an exception that, in this case, you are allowed to have a coeff of zero (we could ensure that no flow is set to zero for every year and region). But maybe that's just babying the user a bit; dunno.

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 "primary output" field to processes

3 participants