Fix: Docs wrongly states that NPV is broken#1214
Conversation
NPV is now not broken and users are free to use it, though in the docs for input files it wrongly states that NPV is broken. Update docs accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
=======================================
Coverage 89.31% 89.31%
=======================================
Files 57 57
Lines 7974 7974
Branches 7974 7974
=======================================
Hits 7122 7122
Misses 555 555
Partials 297 297 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates repository documentation and inline comments to remove outdated statements that NPV support is “broken”, aligning user-facing guidance with current behavior.
Changes:
- Removes the stale “NPV is broken” reference from an investment-selection comment.
- Updates the agent objectives input schema docs to allow
npvwithout warning text and lightly reflows notes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/simulation/investment.rs |
Removes outdated NPV “known issue” reference from an inline comment in the investment selection loop. |
schemas/input/agent_objectives.yaml |
Updates schema notes for objective_type and reflows years notes to remove “NPV broken” warning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if all options have zero capacity. If so, we cannot meet demand, so have to bail | ||
| // out. | ||
| // | ||
| // This may happen if: | ||
| // - the asset has zero activity limits for all time slices with | ||
| // This may happen if the asset has zero activity limits for all time slices with | ||
| // demand. | ||
| // - known issue with the NPV objective | ||
| // (see https://github.com/EnergySystemsModellingLab/MUSE2/issues/716). | ||
| if outputs_for_opts.is_empty() { |
There was a problem hiding this comment.
Heh, good point. It's not necessarily about having zero capacity anymore... I'll tweak this
There was a problem hiding this comment.
I decided to rename the sort_apparisal_outputs_by_investment_priority function to make it clear it also does filtering and move comments around so it's obvious when/why we're discarding non-feasible options
Description
NPV is now not broken and users are free to use it, though in the docs for input files it wrongly states that NPV is broken. Update docs accordingly.
Also update a stale comment saying the same thing.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks