Skip to content

Conversation

@dhensle
Copy link
Contributor

@dhensle dhensle commented Dec 30, 2025

Fix for #820: If zones close, they now remain closed.

@jpn-- jpn-- self-requested a review January 6, 2026 19:15
Copy link
Member

@jpn-- jpn-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Here's why I like it:

  • You have written a clear and simple test that creates a tiny model, runs it, and fails on the existing code in a manner that clearly demonstrates the existing problem and runs in under one second.
  • After the code changes are made, the test passes. Voila!

The one thing that's missing is a note on the change log activitysim/docs/dev-guide/changes.md. This is not really what may of us would call a "breaking change" as it should not formally cause any existing model to fail, but it does break strict reproducibility between versions. This is seen in the SANDAG test, which now fails because the shadow-priced location choices are changing just a tiny bit.

@jpn-- jpn-- added this to Phase 11 Jan 8, 2026
@jpn-- jpn-- moved this to Under Review in Phase 11 Jan 8, 2026
@dhensle
Copy link
Contributor Author

dhensle commented Jan 8, 2026

Great! Glad you like the test -- it was not a trivial exercise to get a nice CI test built for this particular issue. And we also now have a CI test for the simulation-based constraint mechanism as a whole as a bonus!

I have updated the change log. I think this is ready to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Under Review

Development

Successfully merging this pull request may close these issues.

3 participants