-
Notifications
You must be signed in to change notification settings - Fork 8
ABM3 v15.2.1 Production Model Specifications
#34
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: main
Are you sure you want to change the base?
Conversation
|
So obviously the tests for this model are failing, because they are integration tests which rely on the model being the same, and in this PR we are changing the model. We can re-generate the test targets, which will make the tests pass as long as the reproducible random is working, but that will not verify anything is “correct”. @JoeJimFlood have you or anyone at SANDAG reviewed these changes to check they are actually consistent with your production model? |
jpn--
left a comment
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.
I found a few oddities.
| Trip mode is drive,drive_trip,"df.trip_mode.isin(['DRIVEALONE', 'SHARED2', 'SHARED3'])" | ||
| # putting all trips into the same parking segment,, | ||
| Parking segment,parking_segment,'no_segmentation' | ||
| Parking segment,parking_segment,"np.where(df['tour_id'] % 3 == 0, 'segment_1',np.where(df['tour_id'] % 3 == 1, 'segment_2','segment_3'))" |
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.
What is up with this? It looks like we are creating 3 segments in this model by dividing tours almost arbitrarily, but looking at the edits to the coefficients file it appears all the segments are identical.
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.
Segmentation was added to overcome a memory issue - SANDAG/ABM#132. I recall we temporarily overcame this but started seeing crashes again for future year runs (that had thousands of parking constrained locations). Segmentation was increased to 4 recently to be reliably avoid the memory error - https://github.com/SANDAG/ABM/commits/main/src/asim/configs/resident/parking_location_choice.csv.
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.
Ok, I will award points to the house of whomever did this as a clever hack to reduce memory usage. But we do have an (explicit) chunking mechanism for exactly this. Why not use that?
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.
We had to do this because the explicit chunking was not yet implemented and SANDAG needed to be able to run their model. I think explicit chunking is the correct long-term solution.
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.
Since explicit chunking is the superior long-term solution, our options are:
- Revert this change, as memory is not an issue in the example model.
- Add the explicit chunking setting to the example model, even if it diverges from SANDAG’s
v15.2.1production version. - Accept this change and be consistent with
v15.2.1. (sounds the least favored?)
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.
I am ok with option 1 here. I feel that in general, it would be reasonable for these kinds of differences to exist between SANDAG-production and SANDAG-example models.
| util_is_student,Student status,@df.is_student,0,coef_is_student | ||
| utils_sub_asc,Constant,1,0,coef_sub_asc | ||
| util_availability,Availability of transit pass,transit_subsidy_available,0,coef_unavailable | ||
| util_availability,Availability of transit pass,transit_subsidy_available==False,0,coef_unavailable |
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.
This seems counterintuitive, to flip the meaning of the variable name. Also, in the YAML file for this component, we are filtering choosers based on transit_subsidy_available so this should probably never be triggered?
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.
Correct, this is probably never triggered. Not exactly sure why this was added but looking at the history (https://github.com/SANDAG/ABM/commits/main/src/asim/configs/resident/transit_pass_subsidy.csv), it may have been a routine addition for controlling availability.
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.
I checked the source code and it is indeed being filtered so that line in the spec file is redundant.
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.
we went ahead and dropped this expression SANDAG/ABM@a1969da
Some This raises a question: Should we continue carrying over calibration-related (or other minor) changes into the example model, knowing they likely cause integration test failures due to result shifts? Reasons to skip:
Reasons to include:
Open question: |
It's been a few months since I did my review of this, but if I recall correctly I did compare what was in it to the state of our config files. By the time I did my review |
@i-am-sijia I think you raise an important question. I would vote for keeping the integration tests. In general, maybe we do not update the example model with every little calibration update in the production model. In future, maybe we do a "calibration changes only" PR to the example model which would also include updating test targets. The review of this PR needs to ensure that only calibration-related commits are included. We could do this at some set frequency like once per year. |
|
I would like to request this PR be updated to match ABM3 v15.3.1 specs. SANDAG ABM3 v15.3.1 will be released next week and we will essentially be freezing that version for our 2025 rtp (to be adopted in Dec). This would be a good version for the example model to match via this PR. The marginal effort to match v15.3.1 should be minimal and SANDAG can take that on if needed. |
Note: This PR is a duplicate of PR #29, which was auto-closed by GitHub and could not be reopened. The original PR #29 has been reviewed by SANDAG.
This PR brings in model specifications from SANDAG's ABM3 production model, version
v15.2.1.The model specifications changes have been manually reviewed to ensure they do not overwrite any existing Sharrow-related modifications implemented in this repository but not present in SANDAG's production model
v15.2.1.Specifications changes from
v15.2.1include: