Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 135
Add global option to skip households on simulation failure#1023
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
a0ec6a88623516397f250fa08f11f26cc8076fd83396e73eccf98cd2baa47fa6cffd9b0e34b7d5da971550034fda6ed5cb91227b6c986e5be6a8c1bee529165316890f0a2581f04bf149d3d01806c6d75c257e3ca734c16d28faa071cc5fddcb28640615cfc93c3ba996e01f9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1043,6 +1043,41 @@ def force_escortee_trip_modes_to_match_chauffeur(state: workflow.State, trips): | ||
| f"Changed {diff.sum()} trip modes of school escortees to match their chauffeur" | ||
| ) | ||
| # trip_mode can be na if the run allows skipping failed choices and the trip mode choice has failed | ||
| # in that case we can't assert that all trip modes are filled | ||
| # instead, we throw a warning about how many are missing, and return early | ||
| if state.settings.skip_failed_choices: | ||
i-am-sijia marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| missing_count = trips.trip_mode.isna().sum() | ||
| if missing_count > 0: | ||
| # check if the missing trip modes are all because of simulation failures | ||
| # i.e., they are from households that are in the skipped_household_ids set | ||
| import itertools | ||
| skipped_household_ids_dict = state.get("skipped_household_ids", dict()) | ||
| skipped_household_ids = set( | ||
| itertools.chain.from_iterable(skipped_household_ids_dict.values()) | ||
| ) | ||
| missing_household_ids = set( | ||
| trips[trips.trip_mode.isna()]["household_id"].unique() | ||
| ) | ||
| # log a warning about the missing trip modes for skipped households | ||
| missing_count_due_to_sim_fail = len( | ||
| trips[ | ||
| trips.trip_mode.isna() | ||
| & trips.household_id.isin(skipped_household_ids) | ||
| ] | ||
| ) | ||
| logger.warning( | ||
| f"Missing trip mode for {missing_count_due_to_sim_fail} trips due to simulation failures in trip mode choice, " | ||
| f"these records and their corresponding households are being skipped: {missing_household_ids}" | ||
| ) | ||
| # throw assertion error if there are missing trip modes for households that were not skipped | ||
| assert missing_household_ids.issubset(skipped_household_ids), ( | ||
| f"Missing trip modes for households that were not skipped: {missing_household_ids - skipped_household_ids}. " | ||
| f"Missing trip modes for: {trips[trips.trip_mode.isna() & ~trips.household_id.isin(skipped_household_ids)]}" | ||
| ) | ||
| return trips | ||
| assert ( | ||
| ~trips.trip_mode.isna() | ||
| ).all(), f"Missing trip mode for {trips[trips.trip_mode.isna()]}" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -778,11 +778,27 @@ def _check_store_skims_in_shm(self): | ||
| check_model_settings: bool = True | ||
| """ | ||
| run checks to validate that YAML settings files are loadable and spec and coefficent csv can be resolved. | ||
| run checks to validate that YAML settings files are loadable and spec and coefficient csv can be resolved. | ||
| should catch many common errors early, including missing required configurations or specified coefficient labels without defined values. | ||
i-am-sijia marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| """ | ||
| skip_failed_choices: bool = True | ||
| """ | ||
| Skip households that cause errors during processing instead of failing the model run. | ||
| .. versionadded:: 1.6 | ||
| """ | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need additional setting[s] to set thresholds for how many skips are OK and when it's too many and should be an error. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added | ||
| fraction_of_failed_choices_allowed: float = 0.1 | ||
| """ | ||
| Threshold for the fraction of households that can be skipped before failing the model run, | ||
| used in conjunction with `skip_failed_choices`. | ||
| We want to skip problems when they are rare, but fail the run if they are common. | ||
| .. versionadded:: 1.6 | ||
| """ | ||
| other_settings: dict[str, Any] = None | ||
| def _get_attr(self, attr): | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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.
Note that the sample_rate is set to only 3 digits of rounding. This means that for cases where we are running, say, only 100 households to test some spec development on a region with 1.2M households, you will likely get a divide by 0 error here. I suggest we increase the rounding to something much more than 3 and also put in a bit of divide-by-zero protection in this block.
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.
That's a good point. I will add that. Thanks!
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-sijia I think the divide-by-zero problem has been reduced by setting the rounding to 7 instead of 3, but the problem still potentially exists here; the sample_rate could still in some cases be zero. Can we do a np.where or similar to solve it?
Also: why are we rounding this at all? It seems to me it doesn't hurt us to just store and use the value as a float.
Uh oh!
There was an error while loading. Please reload this page.
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 think skip rounding and directly use the value is the better solution.
sample_rateis afloat64regardless of the number of decimals.