-
Notifications
You must be signed in to change notification settings - Fork 46
Phase 10A Populationsim Updates #192
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: master
Are you sure you want to change the base?
Conversation
…imultanousListBalancer.
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 removes legacy example scripts and seed data, modernizes documentation and packaging, and adds IDE and CI configurations.
- Removed outdated example run scripts and seed data CSVs
- Updated documentation formatting, renamed config keys, and added CLI usage to README
- Added VSCode settings, pre-commit hooks, Python version pinning, and GitHub Actions CI
Reviewed Changes
Copilot reviewed 192 out of 192 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
example_test/run_populationsim.py | Removed old example runner script |
example_test/data2/seed_persons.csv | Deleted legacy seed persons data |
example_test/data2/seed_households.csv | Deleted legacy seed households data |
example_test/configs2/controls.csv | Deleted legacy controls configuration |
example_survey_weighting/run_populationsim.py | Removed survey-weighting example runner |
example_calm_repop/run_populationsim.py | Removed CALM repop example runner |
example_calm/run_populationsim.py | Removed CALM example runner |
docs/validation.rst | Trimmed trailing blank lines |
docs/software.rst | Removed trailing whitespace |
docs/conf.py | Reformatted quotes and list syntax |
docs/application_configuration.rst | Renamed column_map to rename_columns |
README.md | Added CLI usage section |
MANIFEST.in | Removed graft directives for example and data files |
.vscode/settings.json | Added VSCode testing and linter settings |
.vscode/launch.json | Added VSCode debug configuration |
.python-version | Pinned Python version to 3.12 |
.pre-commit-config.yaml | Introduced pre-commit hooks for formatting and linting |
.github/workflows/python-package.yml | Added CI workflow for multi-version testing |
Comments suppressed due to low confidence (4)
README.md:21
- The README references an
examples/
directory, but the legacy example scripts and data have been removed. Either update the path to the new examples location or remove this reference to avoid confusion.
See the [examples directory](examples/) for more information on using the command-line interface.
.github/workflows/python-package.yml:25
- [nitpick] Using
actions/setup-python@v4
is more standard and widely supported thanastral-sh/setup-uv
. Consider switching to the official action to simplify setup and improve community familiarity.
uses: astral-sh/setup-uv@v5
.vscode/settings.json:1
- [nitpick] Committing IDE-specific configuration can clutter the repository and affect other contributors. Consider adding
.vscode/
to.gitignore
or isolating personal settings.
{
This reverts commit 4474f60.
Pull Request Testing FeedbackMy colleague and I reviewed the pull request code, examples, testing, and documentation. Additionally, I tested the functionality with an independently developed set of inputs. This scenario, referred to as NCTCOG, has a household sample size of 141,851. So, an order of magnitude larger than the provided examples, but not large enough to encounter convergence failure under the default parameters and the accompanying inconsistent results issues that have been noted by others. We have a document containing our full set of comments, but I'm just posting our primary findings below to limit the length of this comment. Findings
|
I started reviewing the code today. I focused first on documentation and examples. I'll save the core code until tomorrow. I'm a little afraid I'm going to get sidetracked, so I figured it best to drop comments as I go. Initial Thought on the CLI: I like the move to more standardized CLI functions and the use of the TOML file. I was able to create a new environment and run the samples within a matter of minutes. Sphinx DocumentationSimilar to the experience of the @j-murray1, I had several issues with the documentation being seemingly out of date and likely not updated against the most current versions of Sphinx. I had several missing references and a malformed table error when I attempted to build the docs. More importantly, given the scale of the changes to the codebase, it looks like the only changes in the docs folder are cosmetic lint-er fixes (e.g., single-quote to double-quote, linefeeds, spacing). Given the scale of the changes, it would seem appropriate to update the documentation too. A first pass by an AI agent like Claude or Copilot could likely go a long ways. Examples
(popsim) ➜ example_test git:(fork/RSGInc/develop) ✗ populationsim -m 4
Configured logging using basicConfig
INFO:populationsim:Configured logging using basicConfig
INFO:populationsim.run:adding 'configs_mp' to config_dir list...
/Users/cdaniels/apps/activitysim/populationsim/populationsim/__main__.py:33: FutureWarning: Support for 'run_list' settings group will be removed.
The run_list.steps setting is renamed 'models'.
The run_list.resume_after setting is renamed 'resume_after'.
Specify both 'models' and 'resume_after' directly in settings config file.
sys.exit(run(args))
Error: Don't expect 'steps' in run_list and 'models' as stand-alone setting! Overall Note on Examples
I'll add more notes on the actually software changes in the coming days. |
Recommendation: Approve with documentation enhancements and example fixes The Numba performance optimizations deliver impressive results with 2.6x speedup in core balancing algorithms in my tests while maintaining statistical accuracy. Code functionality is good and ready for production. Testing validates both technical implementation and algorithmic consistency across various scenarios. Key Value Delivered:
The comprehensive testing below reveals valuable insights about system behavior that would benefit from documentation to help future users optimize their workflows. Testing Environment & ApproachTest Configuration:
Note: Motionworks runs PopulationSim per individual block group with 75 control targets rather than multi-level configurations. Test Results SummaryTest 1: Core Functionality Validation ✅Objective: Verify system stability and end-to-end pipeline execution Results: Complete success with excellent performance metrics
Timing Breakdown:
Validation: Confirms stable algorithm performance and memory management for production workloads. Test 2: Numba Performance Impact 🚀Objective: Quantify performance improvements from Numba JIT compilation Configuration Changes:
Performance Results:
Key Finding: Numba delivers substantial performance gains with minimal algorithmic impact. The computational bottleneck (initial balancing) sees 2.6x improvement, halving total processing time. Additional Insight: Counter-intuitively, testing with Test 3: Monte Carlo Behavior Analysis 🎲Objective: Assess statistical consistency and randomness across multiple runs Test Setup: 36 independent runs with different Statistical Level Results:
Record Level Results:
Assessment: PopulationSim demonstrates ideal Monte Carlo characteristics:
This validates the system's suitability for both reproducible production use and uncertainty analysis applications. Test 4: Geographic Boundary Effects 📍Objective: Determine if additional GEOIDs affect individual zone outputs Test Setup:
Key Finding: Adding additional GEOIDs significantly affects the target GEOID's synthetic population despite identical control targets. Impact Quantification:
Selected Control Variations:
Root Cause: PopulationSim's initial balancing optimizes across all GEOIDs simultaneously. When additional zones are included, seed households compete globally, resulting in different optimal solutions that satisfy all zone targets collectively. Architectural Insight: The system lacks zone-level independence - synthetic populations for any geography depend on the broader modeling context. This is algorithmically sound but represents important behavior for users designing geographic scopes. Technical Observations for Future EnhancementConfiguration Access PatternsObservation: Mixed usage patterns between Examples:
Impact: Potential configuration mismatches if Pipeline DependenciesDiscovery: The pipeline configuration appears highly flexible but contains undocumented hard dependencies between steps. Critical Dependencies:
Error Example: Commenting out Documentation Enhancement OpportunitiesBased on testing insights, several areas would benefit from documentation additions: 1. Performance Configuration GuideDocument the new Numba-related settings and their performance implications: # Performance Optimization Settings
USE_NUMBA: True # Enable JIT compilation (2.6x speedup)
NUMBA_PRECISION: 'int64' # Numerical precision (int64 fastest in testing) 2. Poorly Documented Configuration VariablesSeveral settings lack clear documentation:
3. Monte Carlo Behavior GuideExplain the
4. Geographic Boundary EffectsDocument how geographic scope affects individual zone results:
5. Pipeline Step DependenciesCreate explicit dependency documentation:
Conclusions & RecommendationsPrimary Recommendation: Approve PR #192 - the Numba optimizations provide substantial performance benefits with maintained algorithmic integrity. Secondary Recommendation: Enhance documentation to capture the valuable behavioral insights revealed during testing. This would help future users:
Offer: I'm happy to contribute documentation updates that incorporate these testing findings - either as an addendum to this PR or as a separate documentation enhancement PR. The performance improvements are excellent and the code is production-ready. The testing insights simply provide an opportunity to make this valuable functionality even more accessible to the user community. |
Thanks @j-murray1 @danielsclint for the review! I've done my best to distill the above and the items from the word doc to populate the PopulationSim development priority board @jpn-- @dhensle I estimated the level of effort with this rough framework in mind:
I left the priority fields blank to be determined. As you all can see I did not touch the documentation, so I think that seems like the minimum place to start. I also definitely welcome any help, documentation or otherwise (@danielsclint ). We can sort out a clean way to collaborate, separate PRs or otherwise. |
Here is the "full text" of TTI's review. |
This is a substantial revision to the PopulationSim code. Major changes include:
Issues Addressed: