-
Notifications
You must be signed in to change notification settings - Fork 99
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
Explicit chunking on all interaction simulate models #870
Conversation
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 looks great. Couple minor changes to simplify.
activitysim/core/chunk.py
Outdated
@@ -1232,12 +1232,19 @@ def adaptive_chunked_choosers( | |||
|
|||
chunk_tag = chunk_tag or trace_label | |||
|
|||
num_choosers = len(choosers.index) | |||
|
|||
explicit_and_odd_num_choosers = False |
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.
The value of explicit_and_odd_num_choosers
is not needed. It's perfectly fine for some multiple of rows_per_chunk
to overrun the end of the choosers by a bit, slicing beyond the end of the range. If it were needed, checking for odd wouldn't be enough, we'd need to adjust based on the inverse of the number of chunks (e.g. 0.25 won't line up unless the total is divisible by 4 not 2)
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.
Please take a look at the changes in this commit: dhensle@1ed94c5
I was hitting the assert statement for the alts, (not the choosers) which prompted me to try to adjust the rows_per_chunk
. But good point about odd not being good enough. I removed this functionality and replace the assert statement with a simple check on overflow of the alt index. I think the solution in the above commit fixes the issue (it runs successfully), but suggest you take a look. Thanks!
activitysim/core/chunk.py
Outdated
& (i == estimated_number_of_chunks) | ||
& (rows_per_chunk > 1) | ||
): | ||
# last chunk may be smaller than chunk_size due to rounding error |
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 don't need to update the rows_per_chunk here, as noted above we can overrun the end of the choosers and be fine.
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.
see above response.
Adds the option of explicit chunking to all interaction simulate models that were not already hooked-up. These include destination choice, location choice, and scheduling.
Also implemented a feature where the explicit_chunk setting can be less than 1. If less than one, it specifies the fraction. So
explicit_chunk: 0.1
would mean that there would be 10 chunks. If greater than 1, explicit_chunk remains the total number of rows in the chooser table.