Skip to content
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

Parking location bug fix #622

Merged
merged 4 commits into from
Dec 21, 2022

Conversation

i-am-sijia
Copy link
Contributor

This PR is to address the bug in parking location choice documented in #620.

The bug fix was done as part of the MTC TM2 ActivitySim implementation. It will be reviewed and adopted by SANDAG ActivitySim implementation. @kulshresthaa made the original code changes and his commit history can be found here.

@i-am-sijia i-am-sijia marked this pull request as ready for review November 2, 2022 18:09
@jpn-- jpn-- self-requested a review November 22, 2022 17:20
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.

So, this code change appears consistent with the intent and from my review of the results in the linked BayDAG repo, it appears to fix the problem.

But: this PR changes the parking location choice model, without touching a single thing in the ARC example implementation -- no config.yaml changes, no spec.csv changes, and, most of all, no changes to any ARC test setup or regression test outcome. And yet, all the tests still pass. This suggests to me that (a) this code change is effectively not being tested adequately in this new PR, and (b) the parking location choice model is probably not being adequately exercised by any test prior to this PR either.

@i-am-sijia
Copy link
Contributor Author

i-am-sijia commented Dec 15, 2022

So, this code change appears consistent with the intent and from my review of the results in the linked BayDAG repo, it appears to fix the problem.

But: this PR changes the parking location choice model, without touching a single thing in the ARC example implementation -- no config.yaml changes, no spec.csv changes, and, most of all, no changes to any ARC test setup or regression test outcome. And yet, all the tests still pass. This suggests to me that (a) this code change is effectively not being tested adequately in this new PR, and (b) the parking location choice model is probably not being adequately exercised by any test prior to this PR either.

We did not need to update the parking location UEC for the actual ARC implementation, the code change fixed the bug. The parking location results after this update matches ARC's CTRAMP implementation, as Raghu noted in his follow-up comments in #620.

I took a look at the prototype ARC trip list, there are no trips having actual parking zone values - they all park at the destination zones - both before and after this change, which explains why the tests still pass. To your point, parking location choice model is not being adequately tested with the current prototype model.

@jpn-- jpn-- merged commit e11b48a into ActivitySim:develop Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants