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

Changed Categorical to work with multidim p at the logp level reloaded #3386

Merged
merged 12 commits into from
Feb 28, 2019

Conversation

lucianopaz
Copy link
Contributor

"Reopening" #3383 with the dimshuffle comment from @junpenglao.

@lucianopaz
Copy link
Contributor Author

@twiecki, issue #3141 (in particular this reply) shows a potential model where the no normalization condition could be dangerous. Maybe I could still force the normalization here for backwards compatibility. What do you think?

@twiecki
Copy link
Member

twiecki commented Feb 27, 2019

@lucianopaz I think auto-normalization is a good idea.

@lucianopaz
Copy link
Contributor Author

@lucianopaz I think auto-normalization is a good idea.

Ok, this change means that the sumto1 check can be dropped.

@lucianopaz
Copy link
Contributor Author

lucianopaz commented Feb 28, 2019

Ok, I think this should now be ready, @twiecki @junpenglao . In the end

  • Categorical.p is not normalized inside __init__.
  • Categorical.logp checks the raw p for negative values and returns -inf if any p entry is negative (does not include zero p).
  • Categorical.logp will always normalize the elements of p to sum 1.
  • Categorical.mode and hence its associated default test points will have shape equal to p.shape[:-1] . If this shape were (1,), it is squeezed.

These changes end up reaching many open issues:

@twiecki twiecki merged commit c613106 into pymc-devs:master Feb 28, 2019
@twiecki
Copy link
Member

twiecki commented Feb 28, 2019

This is fantastic, thanks for the careful work, also in identifying the appropriate issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants