- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.1k
          Add ZeroSumNormal distribution
          #6121
        
          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
Conversation
        
          
                pymc/distributions/continuous.py
              
                Outdated
          
        
      |  | ||
| return super().dist([sigma], zerosum_axes=zerosum_axes, **kwargs) | ||
|  | ||
| # TODO: This is if we want ZeroSum constraint on other dists than Normal | 
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.
@lucianopaz @aseyboldt does any of the math require the summed distribution to be a Normal or could it be something else (e.g., StudentT)?
| Codecov Report
 
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #6121      +/-   ##
==========================================
+ Coverage   93.05%   93.40%   +0.35%     
==========================================
  Files          91      100       +9     
  Lines       20804    22138    +1334     
==========================================
+ Hits        19360    20679    +1319     
- Misses       1444     1459      +15     
 | 
| Thanks for the first review @ricardoV94 ! | 
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.
This would make more sense in multivariate even though we are treating it as a scalar as a hack for the time being.
| 
 Good point. I moved it to  I also added all the tests mentioned and pushed everything. They pass locally. Let's see if they pass 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.
@AlexAndorra i left a bunch of comments all around the place. I think that I’m missing the part where the RV asserts that the zero sum axis are all negative and non repeating. I also think that you need to add some tests for the logp of the distribution and for the variance of the draws
|  | ||
|  | ||
| def extend_axis(array, axis): | ||
| n = array.shape[axis] + 1 | 
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 ?
ZeroSumNormal distribution 🔥ZeroSumNormal distribution
      Co-authored-by: Ricardo Vieira <[email protected]>
9ef5240    to
    3e72922      
    Compare
  
    
This PR introduces the world famous
ZeroSumNormaldistribution, i.e a Normal distribution where one or several axes are constrained to sum to zero. By default, the last axis is constrained to sum to zero.The
zerosum_axesare always in the rightmost position, i.ezerosum_axes=2means the two rightmost axes will be constrained to sum to zero (see examples below for more details).sigmahas to be a scalar, to ensure the zero-sum constraint. The ability to specifiy a vector ofsigmamay be added in future versions.Checklist
get_support_shapewith 2DExamples:
Major / Breaking Changes
Bugfixes / New features
ZeroSumNormaldistributionDocs / Maintenance