Check that all PACs are inputs or outputs

[tsmbland]

Description

This adds a check in read_process_pacs_from_iter to make sure the PACs for each process are either all inputs or all outputs.

I do this by looping through the processes and creating a flag current_flow_sign which is compared against the commodity flow for each PAC (positive flows represent outputs, negative flows represent inputs). Implicit is the assumption that flows cannot be zero - I don't think we actually have this check anywhere, we we should add this in.

This required passing flows to this function and also read_process_pacs. I had to modify all the tests accordingly, and have added a new test.

Fixes #165

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]

Codecov Report

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

Project coverage is 95.25%. Comparing base (1f6cc7d) to head (a9623a9).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
src/process.rs	90.42%	4 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #273      +/-   ##
==========================================
- Coverage   95.69%   95.25%   -0.44%     
==========================================
  Files          13       13              
  Lines        2461     2549      +88     
  Branches     2461     2549      +88     
==========================================
+ Hits         2355     2428      +73     
- Misses         32       44      +12     
- Partials       74       77       +3     

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

[alexdewar]

This is great, especially for your first Rust PR! Good job.

I've made a small suggestion about the test (mutable variables can be a source of errors) and a few comments, but otherwise this is all good.

[AdrianDAlessandro]

I haven't dug into the logic inside the loops, but have some macro-level questions

[AdrianDAlessandro]

Is this Clone (and the one above) only added for the tests? I'm not sure if we want to make the structs clone-able just for the testing, it opens up cloning as an option in other parts of the code, which we maybe don't want? @alexdewar

[alexdewar]

I hadn't really thought about it from that perspective, but that's an interesting thought. I don't think we'll want instances of these objects outside of the input layer, either with "clone()" or "new()". That said, I think we can catch this kind of broken code at review time.

You possibly could make it a test-only feature by doing something like this:

#[cfg(test)])
#[derive(Clone)]
#[derive(PartialEq, Debug, Deserialize)]

Not sure it's worth the effort though.

[alexdewar]

LGTM!

[dalonsoa]

The logic looks fine, but I'd extract the validation as a separate function to improve the readability and the encapsulation of specific functionality.

[alexdewar]

@tsmbland I think this is ready to merge. There will be minor conflicts with #198, but that one's not finished yet so I think we should just merge this for now.