Remove Default implementation for unit types#1145
Conversation
| /// Note: this should be similar to the "shadow price" strategy, which is also based on marginal | ||
| /// costs of the most expensive producer, but may be more successful in cases where there are | ||
| // multiple SED/SVD outputs per asset. | ||
| /// multiple SED/SVD outputs per asset. |
There was a problem hiding this comment.
Surprised this sort of thing doesn't get flagged by any of the checks, to he honest
There was a problem hiding this comment.
Yeah... it seems in general there aren't any tools that check doc comments, which is a bit annoying.
|
Link checker failing with |
There was a problem hiding this comment.
Pull request overview
This PR removes Default from the project’s UnitType abstraction and from the generated unit wrapper types, to prevent silently defaulting domain-specific quantities (e.g., prices) to zero and to force explicit fallbacks where needed.
Changes:
- Remove
Defaultfrom theUnitTypetrait bound and from thebase_unit_struct!derive set. - Replace
or_default()/unwrap_or_default()usages that relied on unit defaults with explicit insertion/modification or explicit fallback values. - Update investment appraisal test scaffolding to construct
ObjectiveCoefficientsexplicitly (instead of derivingDefaultin tests).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/units.rs |
Drops Default from the UnitType contract and base unit derives. |
src/simulation/prices.rs |
Replaces or_default() accumulation with and_modify/or_insert; fixes a doc-comment line. |
src/simulation/optimisation.rs |
Removes unwrap_or_default() on optional input prices and makes the fallback explicit via branching. |
src/simulation/investment/appraisal/coefficients.rs |
Removes test-only Default derive for ObjectiveCoefficients. |
src/simulation/investment/appraisal.rs |
Updates tests to build ObjectiveCoefficients explicitly. |
src/input/asset.rs |
Removes an unused Default derive on an input CSV raw struct. |
src/asset.rs |
Replaces unwrap_or_default() on missing prices with an explicit MoneyPerFlow(0.0) fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flow.coeff | ||
| * prices | ||
| .get(&flow.commodity.id, &self.region_id, time_slice) | ||
| .unwrap_or_default() | ||
| .unwrap_or(MoneyPerFlow(0.0)) |
There was a problem hiding this comment.
This continues to treat a missing commodity price as zero via unwrap_or(MoneyPerFlow(0.0)). Given the motivation for removing Default on unit types (to avoid silently inferring missing prices as free), consider changing this call path to propagate absence (e.g., return Option/Result or require callers to provide a fallback explicitly at the call site where it’s truly intended).
There was a problem hiding this comment.
I think this is beyond the scope of this PR
There was a problem hiding this comment.
Can you remember why we were treating missing prices as zero here? Are there cases where we might not have prices yet?
There was a problem hiding this comment.
Some commodities genuinely don't have a price (all OTH commodities are unpriced), so it's fair to treat them as free in this case. Other (non-OTH) commodities may have a missing price because there are no producers of that commodity and no potential producers of that commodity in the following year. In this case it's probably NOT a good idea to treat the commodities as free, and this may be responsible for some of the weird prices Adam is describing in #1130
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1145 +/- ##
==========================================
+ Coverage 87.52% 87.54% +0.01%
==========================================
Files 55 55
Lines 7626 7634 +8
Branches 7626 7634 +8
==========================================
+ Hits 6675 6683 +8
Misses 649 649
Partials 302 302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexdewar
left a comment
alexdewar
left a comment
There was a problem hiding this comment.
LGTM! Probably safer to remove this potential footgun.
| flow.coeff | ||
| * prices | ||
| .get(&flow.commodity.id, &self.region_id, time_slice) | ||
| .unwrap_or_default() | ||
| .unwrap_or(MoneyPerFlow(0.0)) |
There was a problem hiding this comment.
Can you remember why we were treating missing prices as zero here? Are there cases where we might not have prices yet?
I think this is cleaner/safer
Fixes #1143