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.
When `None`, it indicates that a valid metric could not be computed.
There was a problem hiding this comment.
Pull request overview
This PR prevents invalid LCOX values (specifically the zero-activity case that previously produced NaN) from breaking investment appraisal by making the appraisal metric optional, and improves memory usage by avoiding deep copies of coefficient data in each appraisal output.
Changes:
- Make
AppraisalOutput.metricoptional and filter invalid appraisal outputs during prioritisation. - Update LCOX calculation to return
Nonewhen total activity is zero (avoiding0/0 -> NaN). - Reduce allocations by sharing objective coefficients via
Rcand passing demand explicitly to debug output writers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/investment/appraisal/coefficients.rs |
Returns shared Rc<ObjectiveCoefficients> to avoid repeated deep copies. |
src/simulation/investment/appraisal.rs |
Makes appraisal metrics optional, adds validity filtering, and refactors construction via a helper constructor. |
src/simulation/investment.rs |
Passes demand through to appraisal debug writing after API change. |
src/output.rs |
Writes appraisal metrics as Option<f64> (empty when invalid) and passes demand into time-slice appraisal output. |
src/fixture.rs |
Updates test fixtures to match new AppraisalOutput structure and Rc coefficients. |
src/finance.rs |
Changes lcox to return Option<MoneyPerActivity> and adds tests for the zero-activity case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Create a new `AppraisalOutput` | ||
| pub fn new<T: MetricTrait>( | ||
| asset: AssetRef, | ||
| results: ResultsMap, | ||
| metric: Option<T>, | ||
| coefficients: Rc<ObjectiveCoefficients>, | ||
| ) -> Self { |
There was a problem hiding this comment.
AppraisalOutput::new is declared pub but its signature takes ResultsMap, which is defined in the private optimisation module. This exposes a private type in a public interface (triggers private_interfaces warnings and makes the constructor unusable outside this module). Consider making this constructor non-public (fn/pub(crate)/pub(super)) or changing the signature to accept only the needed fields (capacity/activity/unmet_demand) without referencing ResultsMap directly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Specifically, it checks whether the metric is a valid value (not `None`) and that the | ||
| /// calculated capacity is greater than zero. | ||
| pub fn is_valid(&self) -> bool { | ||
| self.metric.is_some() && self.capacity.total_capacity() > Capacity(0.0) |
There was a problem hiding this comment.
AppraisalOutput::is_valid only checks metric.is_some() and capacity > 0, but it does not exclude metrics whose value() is NaN. Since compare_approx panics when partial_cmp returns None, a Some(metric) with NaN will still cause a runtime panic during sorting/comparison. Consider extending is_valid (or adding a separate validation step) to also require !metric.value().is_nan() (and possibly is_finite()), which would also make the later doc comment about avoiding NaNs accurate.
| /// Specifically, it checks whether the metric is a valid value (not `None`) and that the | |
| /// calculated capacity is greater than zero. | |
| pub fn is_valid(&self) -> bool { | |
| self.metric.is_some() && self.capacity.total_capacity() > Capacity(0.0) | |
| /// Specifically, it checks whether the metric is present and has a finite numeric value | |
| /// (i.e. not `NaN` or infinite), and that the calculated capacity is greater than zero. | |
| pub fn is_valid(&self) -> bool { | |
| self.metric | |
| .as_ref() | |
| .map(|m| { | |
| let v = m.value(); | |
| v.is_finite() | |
| }) | |
| .unwrap_or(false) | |
| && self.capacity.total_capacity() > Capacity(0.0) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1160 +/- ##
==========================================
+ Coverage 87.48% 87.50% +0.02%
==========================================
Files 55 55
Lines 7660 7674 +14
Branches 7660 7674 +14
==========================================
+ Hits 6701 6715 +14
Misses 657 657
Partials 302 302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 😉.
The way I've implemented this is to make the
metricfield ofAppraisalOutputoptional. When we calculate the different metrics we can set this toNonein the case that a valid metric could not be computed; currently I'm only doing this for LCOX where the total activity is zero. Another option would just be to skip over theAppraisalOutputs where these sorts of errors happen (where capacity is zero, for example) -- which is what I did for #1129 -- but the downside of this approach is that it means these appraisal outputs are just omitted in the output file, which would make analysing these sorts of failures much more painful. Now, if the LCOX value is invalid, it will be represented in the output file as an empty string and the asset will not be considered as an option.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