Conversation
The demand map is the same for all `AppraisalOutput`s for a given appraisal iteration, so there's no need to store it. If many assets are being appraised, it could result in many clones.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where LCOX calculations could result in NaN values when total activity is zero (related to issue #1115), and refactors the investment appraisal code to handle non-viable investment options more cleanly. The refactoring also includes performance optimizations by using reference counting (Rc) for shared data structures.
Changes:
- Modified
lcox()function to returnOption<MoneyPerActivity>instead ofMoneyPerActivity, returningNonewhen total activity is zero to prevent NaN values - Refactored
appraise_investment()to returnResult<Option<AppraisalOutput>>, withOk(None)indicating non-viable investment options (zero capacity or zero activity) - Optimized memory usage by wrapping
ObjectiveCoefficientsinRcto avoid unnecessary deep copies across multipleAppraisalOutputinstances, and removed thedemandfield fromAppraisalOutput(passing it explicitly where needed instead)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/finance.rs | Modified lcox() to return Option and added test case for zero activity scenario |
| src/simulation/investment/appraisal.rs | Added AppraisalOutput::new() constructor that filters zero-capacity results, updated calculate_lcox() and calculate_npv() to handle Option returns, removed redundant zero-capacity filtering from sort function, removed obsolete test, updated return types to Result<Option<AppraisalOutput>> |
| src/simulation/investment/appraisal/coefficients.rs | Wrapped return type in Rc<ObjectiveCoefficients> for shared ownership |
| src/simulation/investment.rs | Updated to handle Option return from appraise_investment(), passes demand explicitly to output writer |
| src/output.rs | Added demand parameter to appraisal result writing functions |
| src/fixture.rs | Updated test fixture to use Rc for coefficients and removed demand field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1129 +/- ##
==========================================
+ Coverage 87.52% 87.54% +0.01%
==========================================
Files 55 55
Lines 7626 7638 +12
Branches 7626 7638 +12
==========================================
+ Hits 6675 6687 +12
+ Misses 649 648 -1
- Partials 302 303 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…than filtering later
alexdewar
force-pushed
the
handle-lcox-errors
branch
from
1fb221f to
d376075
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Returns `None` if the capacity is zero, otherwise `Some(AppraisalOutput)` with the specified | ||
| /// parameters. | ||
| pub fn new<T: MetricTrait>( | ||
| asset: AssetRef, | ||
| results: ResultsMap, | ||
| metric: T, | ||
| coefficients: Rc<ObjectiveCoefficients>, | ||
| ) -> Option<Self> { | ||
| if results.capacity.total_capacity() == Capacity(0.0) { |
There was a problem hiding this comment.
AppraisalOutput::new treats capacity as “zero” only when it is exactly Capacity(0.0). Since total_capacity() is derived from solver output (floating point), this exact-equality check can miss very small/negative values and let effectively-zero-capacity options through. Consider using <= Capacity(0.0) (or an epsilon/approx check consistent with the rest of the module) to decide viability.
| /// Returns `None` if the capacity is zero, otherwise `Some(AppraisalOutput)` with the specified | |
| /// parameters. | |
| pub fn new<T: MetricTrait>( | |
| asset: AssetRef, | |
| results: ResultsMap, | |
| metric: T, | |
| coefficients: Rc<ObjectiveCoefficients>, | |
| ) -> Option<Self> { | |
| if results.capacity.total_capacity() == Capacity(0.0) { | |
| /// Returns `None` if the capacity is zero (or effectively zero / non-positive), | |
| /// otherwise `Some(AppraisalOutput)` with the specified parameters. | |
| pub fn new<T: MetricTrait>( | |
| asset: AssetRef, | |
| results: ResultsMap, | |
| metric: T, | |
| coefficients: Rc<ObjectiveCoefficients>, | |
| ) -> Option<Self> { | |
| if compare_approx(results.capacity.total_capacity(), Capacity(0.0)) != Ordering::Greater { |
| 50.0, 100.0, | ||
| vec![("winter", "day", 0.0)], | ||
| vec![("winter", "day", 0.0)], | ||
| None // (50*0 + 25*0) / 0 = not feasible |
There was a problem hiding this comment.
The new test case’s inline explanation is mathematically incorrect: the fixed-cost term is still annual_fixed_cost * capacity (e.g. 100*50), so the numerator is not zero even though total activity is. Please update the comment to reflect the actual expression being avoided (division by zero leading to an invalid/infinite result).
| None // (50*0 + 25*0) / 0 = not feasible | |
| None // (50*100 + 0*0) / 0 -> division by zero (not feasible) |
| impl AppraisalOutput { | ||
| /// Create a new `AppraisalOutput`. | ||
| /// | ||
| /// Returns `None` if the capacity is zero, otherwise `Some(AppraisalOutput)` with the specified | ||
| /// parameters. | ||
| pub fn new<T: MetricTrait>( | ||
| asset: AssetRef, | ||
| results: ResultsMap, | ||
| metric: T, | ||
| coefficients: Rc<ObjectiveCoefficients>, | ||
| ) -> Option<Self> { | ||
| if results.capacity.total_capacity() == Capacity(0.0) { | ||
| debug!("Skipping investment option with zero capacity"); | ||
| return None; | ||
| } | ||
|
|
||
| Some(Self { | ||
| asset, | ||
| capacity: results.capacity, | ||
| activity: results.activity, | ||
| unmet_demand: results.unmet_demand, | ||
| metric: Box::new(metric), | ||
| coefficients, | ||
| }) | ||
| } |
There was a problem hiding this comment.
New viability logic is centralized in AppraisalOutput::new (skipping zero-capacity options), but there’s no unit test exercising that constructor behavior. Adding a small test that builds a ResultsMap with Capacity(0.0) and asserts AppraisalOutput::new(...) returns None would help prevent regressions (and would replace the removed “filter zero capacity during sort” test with a more direct check).
1 participant
Description
In the course of a discussion about something else (#1115), it transpired that it is possible to end up with a NaN value for LCOX, if the total activity is zero and the denominator is zero, which we don't want. We should panic if any of the metrics is zero, but in this case, the offending asset was filtered out because its capacity was zero anyway. We shouldn't rely on this though 😉.
I've refactored things so that
appraise_investmentnow returnsResult<Option<AppraisalOutput>>instead ofResult<AppraisalOutput>. It returnsOk(None)in the case that the investment is not a viable option, currently either because the returned capacity is zero or, for LCOX, the total activity was zero. In these cases, the appraisal result will no longer be written to file, but I'm not sure that's a great loss, because we are only skipping results with invalid results anyway. It seems worth it not to have inconsistentAppraisalOutputs floating around.Unrelated change: While I was refactoring things, I noticed we are storing deep copies of the remaining demand and objective coefficients in
AppraisalOutput, which is less than ideal. If there are large numbers of assets, this would mean a lot of unnecessary allocations and deallocations. For the demand, the value is the same for everyAppraisalOutputanyway (it's the demand that the appraisal is trying to meet), so I just removed this field and changed things to manually pass this value to the one place it's used (writing output files). For objective coefficients, I took the easy option and wrapped it in anRc, which seemed cleaner than either storing a reference and passing lifetime parameters all over the place or passing the coefficients again where they are used.Fixes #1126.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks