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

Overflow protection for NL #821

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jpn--
Copy link
Member

@jpn-- jpn-- commented Feb 27, 2024

Fixes the crash in #815.

The crash is caused by "underflow", where taking something like exp(-999) results in 0 instead of a positive number so small it cannot be represented as a double precision float. The overflow_protection here also protects against underflow.

This does not prevent a high school student from being assigned to a far-away high school, it merely ensures that a mode can be chosen for trips no matter how bad the utility of the best choice is, as long as it remains a finite value.

I have also added a warning, if the overflow_protection option saves the process from crashing but is picking a choice with utility less than -85, a warning is emitted. I've also added a unit test to confirm this warning is emitted correctly.

@jpn-- jpn-- marked this pull request as draft February 28, 2024 15:49
@jpn--
Copy link
Member Author

jpn-- commented Feb 28, 2024

I've added unit test that confirm that overflow protection on the NL model isn't changing any probabilities, it is just shifting the utility values into a range where exp(utility) doesn't over(under)flow.

The Marin test is presently failing because it includes a check of the trace files in addition to the main results. The trace files on exp(utility) do shift from the overflow protection adjustment, but since ActivitySim is applying NNNL weirdly normalized UMNL it's not simple to counteract this shift in the trace file outputs. I'm going to fix this by regenerating the trace files for the Marin test with the adjustment on.

@jpn-- jpn-- marked this pull request as ready for review March 3, 2024 22:40
@jpn-- jpn-- requested a review from i-am-sijia March 3, 2024 22:40
@jpn--
Copy link
Member Author

jpn-- commented Mar 8, 2024

I am adding settings to model components to allow users to turn overflow protection on or off by component.

Per our discussions, there has been some debate about whether the default for this setting should be True or False. When it is False ActivitySim will "crash" when an overflow or underflow occurs unexpectedly. Some have argued that emitting a warning is not enough, and users or developers might not see the warning, or choose to ignore it.

But let me offer one more argument why the default value of this setting should be True: when users are exploring policies, they may provide inputs to the model that are somewhat outside the range of those used in testing in development, e.g. setting fuel prices at 10X. This could easily trigger an overflow problem where one would not see problems under "normal" operation of the model, and crashing the model in these circumstances isn't the ideal solution. I'll note this situation is not purely hypothetical, I have actually seen this happen, not on an ActivitySim model but on a different travel demand model.

@bettinardi
Copy link

Not wanting to make this too long, but I don't quite follow your argument to have True as the default setting. You provide the example that 10x costs may break the limits of development and cause a crash. Wouldn't setting to True cause the users to blow by that fact without a care in the world. Shouldn't there be some kind of crash to indicate that you have broken the limits of the development and you can proceed if you like by setting to TRUE...

If this is the case, I suggest a nice warning message at the fail point that states for the user that the default is false to let them know they have crossed a development limit, but provides quick (in the moment) instructions for where to change the setting to True if the choose to press on.

@jpn--
Copy link
Member Author

jpn-- commented Mar 8, 2024 via email

@i-am-sijia
Copy link
Contributor

I still vote for the default of overflow protection to be False. Most of the times models crash due to either input error (e.g., disconnected zones due to network error), or specification error (e.g., the num_escortees issue #814 ). This happens in the base year model development, and also in the future year scenario development. Defaulting overflow protection to True and having model run pass under these circumstances is not good.

In cases where we do want some sort of overflow protection, there is another solution, which is implemented in CT-RAMP2 models. When a household is failing in the simulation, skip it and exclude it in the subsequent components and exclude it in the output. Keep a list of such households that failed and write them out, give the user a warning in the end with the number of households that failed. If it's just a small number of them, yes it's probably an edge case and user can choose to ignore them and trust the model result. But if it's a large number of households that failed, then it suggests bigger issues like inputs or specifications, then user can investigate. This solution allows the model run to complete, at the same time gives user information on the scale of potential issues behind "the overflow".

@i-am-sijia
Copy link
Contributor

Based on the mtg today, we agree this PR has less priority in Phase 9A. This is not the root cause/solution for #814. But in the long run, if we want to implement some overflow protection, the members prefer some solution close to my comment above ⬆️.

@jpn-- jpn-- marked this pull request as draft March 26, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

None yet

3 participants