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.
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
ZeroSumNormal
distribution #6121Add
ZeroSumNormal
distribution #6121Changes from 13 commits
6260c84
af96016
71e5651
3cadb26
a66c586
e3be495
759de36
a5a1e45
c9eea6e
0582d7c
0bdcdd7
854ef4c
fd3aefa
e94e4f1
dec4a9f
f7a55c5
da6eaab
a5ed1f0
126e76b
3a8d898
4c52737
7e4ed0a
44b5b91
99dbb38
e3dc1d4
09f0d91
cf5b384
3e86a3e
ce68f02
09d849c
b50909e
c204131
7ba1d0f
5ee950a
95ffc94
13a54e6
ca655bc
9d419ef
85da56c
f363118
64eca5c
c5e76c9
08c9df0
c120f7e
ba5f3a1
48dafe9
6612a24
6b07a2a
135ed47
cba0187
566f308
5954e65
3e72922
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think we might want to handle the case where
zerosum_axes=[]
?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.
Isn't that just a Normal distribution?
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.
It is, but if you write more general code that somehow produces the
zerosum_axes
automatically, that could easily happen I think.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.
Oh so you mean erroring out in that case, gotcha
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.
Well, erroring out would be ok, but can't we just handle that case correctly? I can't think of anything that should go wrong in this case. So why not just
And maybe add a test that checks if
pm.ZeroSumNormal("y", zerosum_axes=[], shape=(3,))
or so works and gives us a normal dist.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.
Oh ok, I understand what you mean now. I'm curious what @ricardoV94 thinks, but I would prefer not to do that: if people wanna use a Normal they should just use
pm.Normal
, otherwise I find it makes code and models more confusingThere 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 I agree with you now @aseyboldt (mainly because that behavior would be consistent with other PyMC distributions' behavior).
However, this edge-case needs a bit more work right now, because
at.stack
inget_support_shape
fails with an empty tuple.As it's not a core feature of ZSN, I'm marking this as TODO. The code is actually already there, so if someone needs this in the future, there are already some breadcrumbs to get started.
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.
We could maybe add a comment here saying that this is using a householder reflection plus a projection operator to move forward from the constrained space onto the zero sum manifold. I’ll look up our notes and write something here
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.
Did you find your notes @lucianopaz ?