-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix unit test and posteriors #232
Conversation
23a3364
to
6394cdf
Compare
6394cdf
to
05fb8c6
Compare
05fb8c6
to
a996c2e
Compare
I coded it up so that the The conversion to probabilities is then done immediately after the posteriors are returned, in the |
a996c2e
to
cb82cdc
Compare
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.
LGTM! Only a small semantic quibble regarding "normalised" (I'd try to avoid using as an adjective in the docs when talking about anything that's rescaled but doesn't sum/integrate to 1), and a question regarding the overflow protection.
**Bugfixes** | ||
|
||
- The returned posteriors when ``return_posteriors=True`` now return actual | ||
probabilities (scaled so that they sum to one) rather than normalised |
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 use of "normalised probabilities" is a bit confusing here, because normalizing a probability distribution typically means scaling such that it sums/integrates to 1; not the sort of under/overflow-protection that is happening here
Thanks a lot. I agree about "normalisation" (I'm not sure I named that one). I think "standardised" would be better. I reckon I should change the name of the method to that. |
Previously we claimed to return posteriors that were probabilities but the testing was broken, so we didn't notice that we didn't scale the posteriors to add up to one (so they weren't actually probabilities)
cb82cdc
to
5cc7fad
Compare
Merging. Will tackle renaming "normalise" to "standardize" in a followup PR. Thanks a lot for the feedback @nspope |
Previously we claimed to return posteriors that were probabilities but the testing was broken, so we didn't notice that we weren't scaling the posteriors to add up to one (i.e. they weren't actually probabilities). This fixes the problem, but therefore changes the posteriors that are output. I think this is the right thing to do.