-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add numerically safe ordered probit distribution. #4232
Conversation
Hi @tohtsky thanks for the PR! This looks very useful indeed. As you can see some code checks are failing. Specifically the pre-commit one fails for some style reason (see the details there). If you fix that issue and push, the automatic tests will run again. We're currently in the process of lining up PRs for upcoming release 3.10, this one might still make it I think. |
Codecov Report
@@ Coverage Diff @@
## master #4232 +/- ##
==========================================
- Coverage 87.87% 87.85% -0.02%
==========================================
Files 88 88
Lines 14473 14501 +28
==========================================
+ Hits 12718 12740 +22
- Misses 1755 1761 +6
|
Hi @Spaak , thank you for the comment and sorry I'm not carefully reading the contribution guide. I have fixed the styling issues and made the test more accurate (the first test failed with float32 due to a numerical problem on the scipy side). I would be very happy to receive further comments and suggestions! |
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.
Looks good to me! Apologies for the slow follow-up, we got a bit busy with the 3.10 release and I thought it better to leave this one until after. Thanks @tohtsky!
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.
Actually, before merging, it would be great if you could add a line to the release notes, in the top, the "On deck" section.
Thanks for the review, @Spaak ! |
Thanks again @tohtsky! 🎉 |
This feature adds a ordered probit distribution,
which uses probit link function (i.e., std_cdf) instead of logistic one (sigmoid) used by OrdredLogistic.
Since std_cdf converges to 1 or 0 much faster than sigmoid, additional care must be taken
in order to deal with outlier values without encountering numerical instability,
For example, if we simply use std_cdf instead of sigmoid in the implementation of
OrderedLogistic
,we will be getting
bad initial energy
error in the following example.The implemented version of
OrderedProbit
avoids this issue by carefully using scaled error functionerfcx
.