Unhide NPV and add test using new find-and-replace option #1093
Conversation
We don't use it anywhere.
|
Oops, it seems to be failing because of insufficient demand. Maybe hold off on reviewing for now. |
There was a problem hiding this comment.
Pull request overview
This PR adds a find-and-replace value option to the patching infrastructure and removes the "broken" designation from the NPV (Net Present Value) objective type. The changes enable testing NPV functionality via a new regression test using the simple example model.
Changes:
- Added value-based find-and-replace capability to the patching system
- Removed NPV validation warnings and "broken option" checks from agent objectives
- Added regression test for NPV using the new patching feature
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/regression.rs | Added regression test definition for simple_npv patched example |
| src/patch.rs | Introduced Replacements enum with Values variant to support find-and-replace; refactored FilePatch to use unified replacement mechanism; added unit test for value replacement |
| src/input/agent/objective.rs | Removed NPV-specific validation code and warnings that marked it as a broken feature |
| src/example/patches.rs | Added simple_npv patch definition that replaces "lcox" with "npv" in agent_objectives.csv |
| Cargo.toml | Moved map-macro from dev-dependencies to main dependencies for use in patch.rs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Patched examples | ||
| define_regression_test_with_patches!(simple_divisible); | ||
| define_regression_test_with_patches!(simple_npv); |
There was a problem hiding this comment.
The regression test for simple_npv has been added, but the corresponding test data directory (tests/data/simple_npv/) does not appear to exist. The test data needs to be generated using the regenerate_test_data.sh script and committed to the repository, similar to how tests/data/simple_divisible/ exists for the simple_divisible patch. Without this test data, the regression test will fail.
There was a problem hiding this comment.
Yes, I forgot to do this... I'm impressed that Copilot clocked!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Aurashk
left a comment
Aurashk
left a comment
There was a problem hiding this comment.
Looks good! Just had one thought looking through this.
| } | ||
| } | ||
|
|
||
| /// Structure to hold patches for a model csv file. |
There was a problem hiding this comment.
Might be useful here to document here how these file patches behave when you chain them together. To me it's not obvious whether you can combine addition and deletion with find and replace in an ordered way or you have to pick one.
|
Superseded by #1094. |
Description
I went with @Aurashk's idea of adding a find-and-replace option to the patching code. It wasn't strictly necessary but hopefully we'll find a use for it outside of this one test (otherwise I'll feel silly...).
Anyway, I've added a regression test for
simpleexample + NPV, which was the main thing. I've unhidden the NPV option too.Closes #1062. Closes #1090.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks