Changed Categorical to work with multidim p at the logp level #3383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
junpenglao
reviewed
Feb 25, 2019
pymc3/distributions/discrete.py
Outdated
|
|
||
| if p.ndim > 1: | ||
| a = tt.log(p[tt.arange(p.shape[0]), value_clip]) | ||
| # Fancy trick to index the last axis without touching the other | ||
| a = tt.log((p.T)[value_clip].T) |
There was a problem hiding this comment.
There got to be a better way to do two transpose, right? If it is really necessary, maybe it is easier to do np.swapaxes or np.moveaxes?
There was a problem hiding this comment.
Yup, np.moveaxes is the best option. I was not aware that it worked well with theano tensors too.
There was a problem hiding this comment.
Actually I didnt realized it works with np.moveaxes as well. So it works well with theano tensor i take it - would it be better use tt.dimshuffle? http://deeplearning.net/software/theano/library/tensor/basic.html#dimshuffle
|
Oops, I reverted this commit (de91752) accidentally. Can you reopen? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should close #2082. However, there is one very important point that we need to discuss:
When should
Categorical.pbe normalized to sum to 1?We must do the normalization for backwards compatibility. But normalizing may hide negative values supplied into
p. I did some changes tologpto normalizepthere, but still check for the negative values that could have been set inself.p, but those changes may break earlier codes that relied on the default normalization. I can't come up with a way to handle the edge cases put forward by @kyleabeauchamp, that contained negative values inp, still do the normalization well, and also test for weird values set by step_methods proposals.