Allow flow coeff to be zero

[dalonsoa]

Description

The approach has been to make another check is_zero and use it in combination with is_input and is_output where relevant, in particular when validating the inputs.

I've play around changing to zero several inputs and outputs in the examples and, obviously, sometimes we end up with an unfeasible problem because there's no production to fulfill the demand, but that is the expected behaviour in those cases, not an error.

I've not implemented any validation that flows are not zero for all years since I feel users might want to do that on some scenarios - eg. disabling a particular process to see what happens without having to re-do the input files entirely.

Fixes #928

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

❌ Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.95%. Comparing base (a2b9f19) to head (800e3ce).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simulation/prices.rs	0.00%	3 Missing ⚠️
src/input/process/flow.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #978      +/-   ##
==========================================
- Coverage   84.01%   83.95%   -0.07%     
==========================================
  Files          50       50              
  Lines        5682     5696      +14     
  Branches     5682     5696      +14     
==========================================
+ Hits         4774     4782       +8     
- Misses        679      685       +6     
  Partials      229      229              

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

There seems to be a bit of inconsistency about when to treat zero flows as inputs vs outputs. I don't have all the answers myself, but I think this requires some thought, or at least some of the reasoning here needs more explanation.

If this was me, I might have done this by writing a FlowDirection enum (with values Input, Output and Zero), then replace the is_input()/is_output()/is_zero() methods with a single direction() method. Then, we could use match arms to explicitly deal with the three scenarios where appropriate, and we're not going to inadvertently assume that is_input() -> false necessarily means its an output flow.

If in some circumstances we want zero flows to behave both like inputs and outputs (e.g. validate_secondary_flows), then we could do this by writing a custom Eq implementation for FlowDirection. There could be some unintended consequences of that though, not sure

[tsmbland]

I wouldn't consider zero flows here. If a flow has a coefficient of zero then it shouldn't be a primary output

[tsmbland]

As above

[tsmbland]

Here you're treating zero flows as like inputs. So it will be ok if a flow is negative some years and zero other years, but not ok if it's positive some years and zero other years?

I don't really understand the reasoning for this. I think probably either both scenarios or neither scenarios should be accepted

[tsmbland]

Is this true? At least not according to is_input and is_output

[dalonsoa]

I see your point. I'll revisit using the FlowDirection approach, which is more elegant.

Having said that, I think we need to clarify when we want to allow zero coeff. For your comments, PrimaryOutputs must not be zero under any circumstance. Is this correct? If there are no outputs, do we allow for the inputs to be zero?

[tsmbland]

I see your point. I'll revisit using the FlowDirection approach, which is more elegant.

Having said that, I think we need to clarify when we want to allow zero coeff. For your comments, PrimaryOutputs must not be zero under any circumstance. Is this correct?

It's not necessarily a problem, it might not break anything, although it means that process will never be invested in. I can maybe see a use case for this in wanting to temporarily "silence" a process. That said, I don't think we should infer commodities with zero coeff as the primary output, so this means changing infer_primary_output back to how it was.

If there are no outputs, do we allow for the inputs to be zero?

Probably fine, but that process is essentially a null process. I think in a final model it would be better for users not to include a process like this, but I can see that it could be useful during debugging to temporarily turn off flows, as above.

The graph validation excludes zero flows (as it should), so hopefully if zero coeffs make the model infeasible then it should be picked up at that stage

[tsmbland]

I think this is better, more explicit at least