-
Notifications
You must be signed in to change notification settings - Fork 2
Unhide NPV and add test (take 2) #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
We don't use it anywhere.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1094 +/- ##
==========================================
+ Coverage 85.14% 86.27% +1.13%
==========================================
Files 55 55
Lines 7559 7555 -4
Branches 7559 7555 -4
==========================================
+ Hits 6436 6518 +82
+ Misses 816 730 -86
Partials 307 307 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR re-enables the Net Present Value (NPV) objective for agents and adds an integration-style regression test to lock in the expected behaviour via a patched example model.
Changes:
- Remove the guard that previously blocked
ObjectiveType::NetPresentValuefrom being used in agent objectives. - Define a new patched example
"simple_npv"and hook it into themuse2 exampleCLI patch mechanism and regression test suite. - Add regression output data (assets, commodity flows, and prices) for the
"simple_npv"example and introduce a small internal refactor in the patching code (CSVTable alias).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/input/agent/objective.rs |
Removes the special-case error and warning that treated the NPV objective as broken, allowing objective_type = "npv" to be used like other objectives. |
src/example/patches.rs |
Extends the patch map with a "simple_npv" entry that patches the simple example’s agent_objectives.csv to set A0_RES’s objective to NPV. |
src/patch.rs |
Introduces a CSVTable type alias for IndexSet<Vec<String>> and updates FilePatch and modify_base_with_patch to use it, without changing behaviour. |
tests/regression.rs |
Adds a patched regression test define_regression_test_with_patches!(simple_npv); so muse2 example run simple_npv --patch is checked against golden CSV outputs. |
tests/data/simple_npv/assets.csv |
Provides expected asset state outputs for the patched "simple_npv" example across milestone years. |
tests/data/simple_npv/commodity_flows.csv |
Provides expected per-asset commodity flow outputs for the "simple_npv" example to validate NPV path behaviour. |
tests/data/simple_npv/commodity_prices.csv |
Provides expected commodity prices for all time slices and milestone years for the "simple_npv" example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Sorry, forgot to add PR description before requesting review. I've edited it now. |
Description
My previous attempt at adding a test for NPV didn't work (#1093). @tsmbland pointed out that we could probably get away with just changing the objective type for one agent to NPV, which I've done here and seems to fix things.
This PR just includes the stuff related to NPV (except for a couple of minor tweaks) and I've left the code related to #1090 on a different branch (
replace-value-option-for-patch-examples). I figure we can open a PR for that later if/when we find a use for it.Closes #1062.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks