Repository navigation
Fix for simulation-based shadow pricing segmentation - #1083
Open
i-am-sijia wants to merge 4 commits into
Open
i-am-sijia wants to merge 4 commits into
i-am-sijia wants to merge 4 commits into
Conversation
5 of 10 tasks
Member
|
As discussed in engineering 8/27, this needs a unit test. @i-am-sijia will tackle this. |
i-am-sijia
marked this pull request as ready for review
September 21, 2026 18:41
jpn--
requested review from
bhargavasana and
jpn--
and
a balanced review from Copilot
October 8, 2026 18:18
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Validation includes ineligible persons, and removing a previously supported setting breaks existing configurations.
2 open findings
What changed in this PR
Updates simulation-based shadow pricing to use each location model’s configured chooser segmentation.
Changes:
- Uses
CHOOSER_SEGMENT_COLUMN_NAMEfor resampling. - Adds configuration validation and custom-segment tests.
| File | Description |
|---|---|
activitysim/abm/tables/shadow_pricing.py |
Refactors segmentation and adds validation. |
activitysim/abm/test/test_misc/test_shadow_pricing_simulate.py |
Tests string segments and non-overlapping values. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
129
to
130
| WRITE_ITERATION_CHOICES: bool = False | ||
|
|
|
|
||
| # Fail fast on obvious misconfiguration instead of silently sampling no one. | ||
| segment_values = set(self.segment_ids.values()) | ||
| chooser_values = set(persons_merged[segment_name].dropna().unique()) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressing #1082 with option 2 ...
Summary generated by Copilot (reviewed):
This pull request refactors how chooser segments are handled in the shadow pricing logic, making the configuration more flexible and robust. It uses
CHOOSER_SEGMENT_COLUMN_NAMEfrom the location choice model setting for chooser segmentation to keep shadow-pricing resampling consistent withsegment_idsin location choice settings, adds better error handling for misconfigurations, and introduces new tests to cover these scenarios.Shadow Pricing Configuration Improvements:
SEGMENT_TO_NAMEmapping from both the module-level dictionary and theShadowPriceSettingsclass.ShadowPriceCalculatornow uses theCHOOSER_SEGMENT_COLUMN_NAMEfrom location choice model settings to determine the chooser segment column dynamically.Error Handling Enhancements:
update_shadow_prices, added explicit checks to raise aSystemConfigurationErrorif the configured chooser segment column is missing frompersons_merged, or if there is no overlap betweenSEGMENT_IDSvalues and the values present in the chooser segment column.Testing Improvements: