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

worn in the case of non-convergence #211

Merged
merged 3 commits into from
May 18, 2018
Merged

worn in the case of non-convergence #211

merged 3 commits into from
May 18, 2018

Conversation

Eh2406
Copy link
Contributor

@Eh2406 Eh2406 commented May 4, 2018

At yesterday's meeting with @semcogli and @janowicz discuss how deeply odd our fitted mnl models wore. Specifically, we have variables that have to be positive that are being fit as negative. In the meeting we considered the possibility of collinearity, or spurious correlations, or bad sampling processes, or other complex possibilities. After the meating I thought that the simplest explanation would be if the optimization algorithm had not successfully fitted the models in the first place.

Looking at the code I see that we did not check it optimize.fmin_l_bfgs_b had succeeded. So I added a warning that reports some diagnostic information in case of non-convergence.

When I ran the tests I noted that test_alternative_specific_coeffs reliably thru a non-convergence warning 'ABNORMAL_TERMINATION_IN_LNSRCH'. According to the internet, that diagnostic code is usually caused by programer error. I also noted the RuntimeWarning: overflow encountered in exp. After some research I found a standard technique for avoiding overflow in softmax calculations, applying it solved both warnings. The resulting output did not otherwise change.

TLDR:

  • Warn if we may be returning incorrect output.
  • Fix a numerical instability / Overflow.
  • No evidence that it solves any of the bigger problems.

@coveralls
Copy link

coveralls commented May 4, 2018

Coverage Status

Coverage increased (+0.008%) to 94.345% when pulling 9794d9e on SEMCOG:stability into 2497039 on UDST:master.

fix non-convergence in tests
@Eh2406
Copy link
Contributor Author

Eh2406 commented May 4, 2018

cc @smmaurer looks like this code was copied to https://github.com/UDST/choicemodels/blob/master/choicemodels/mnl.py#L652
When we get this to a good state mabey this pr should be done there as well? Is there other places it has been copied?

@Eh2406
Copy link
Contributor Author

Eh2406 commented May 15, 2018

Ping?

@smmaurer
Copy link
Member

Hi @Eh2406, this looks great! That is super helpful to fix the runtime overflow, and good idea to report if the convergence fails. I'm going to merge this.

You're right that the code was copied to choicemodels -- i think the urbansim version wasn't quite encapsulated enough for us to just call it. And as far as i know that's the only place. Do you want to open a PR there too? (Not trying to generate more work for you, just give you full credit for the fixes. I can also update it myself.)

@smmaurer smmaurer merged commit 79f815a into UDST:master May 18, 2018
@Eh2406
Copy link
Contributor Author

Eh2406 commented May 18, 2018

Thanks! I will make a pr over there soon.

@Eh2406 Eh2406 deleted the stability branch May 18, 2018 17:10
Eh2406 added a commit to SEMCOG/choicemodels that referenced this pull request May 18, 2018
@Eh2406
Copy link
Contributor Author

Eh2406 commented May 18, 2018

I was just ready to make a pr from: SEMCOG/choicemodels@7d7022f

But looks like you did better testing in: https://github.com/UDST/choicemodels/tree/fix-runtime-overflow

So I will let you do it!

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.

3 participants