Skip to content

Add MvNormal moment #5171

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

Merged
merged 14 commits into from
Nov 16, 2021
Merged

Add MvNormal moment #5171

merged 14 commits into from
Nov 16, 2021

Conversation

yadav-sachin
Copy link
Contributor

Add moment and tests for the below distribution as part of #5078

  • pymc.distributions.multivariate.MvNormal

@ricardoV94 ricardoV94 mentioned this pull request Nov 11, 2021
51 tasks
@ricardoV94
Copy link
Member

@yadav-sachin Thanks for opening the PR. I left some comments above. The MvNormal might be a tough nut to crack, as it is not always straightforward to figure out what the right shape should be. Perhaps @michaelosthege will be able to give you some tips

@Sayam753
Copy link
Member

@yadav-sachin The conflicts need to be resolved. Otherwise the changes look good to me 🚀

@codecov
Copy link

codecov bot commented Nov 14, 2021

Codecov Report

Merging #5171 (2595160) into main (38f2632) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5171      +/-   ##
==========================================
+ Coverage   78.07%   78.10%   +0.03%     
==========================================
  Files          88       88              
  Lines       14176    14182       +6     
==========================================
+ Hits        11068    11077       +9     
+ Misses       3108     3105       -3     
Impacted Files Coverage Δ
pymc/distributions/multivariate.py 71.72% <100.00%> (+0.24%) ⬆️
pymc/backends/report.py 91.60% <0.00%> (+2.09%) ⬆️

@Sayam753
Copy link
Member

You can run pre-commit run --all-files locally. Refer to PyMC Python Style Guide in case pre-commit is not configured.

@yadav-sachin
Copy link
Contributor Author

Let me know if any more changes are required. Thanks for the suggestions and reviews.

@yadav-sachin
Copy link
Contributor Author

@ricardoV94 Two checks are failing on this pull request. What should I do to resolve them?

@michaelosthege
Copy link
Member

I think that one failing test is flaky, related to #4771, so don't worry about it.

But I just checked this on my phone, so maybe look into it and double check if it may involve the MvNormal moment

@ricardoV94
Copy link
Member

I think that one failing test is flaky, related to #4771, so don't worry about it.

But I just checked this on my phone, so maybe look into it and double check if it may involve the MvNormal moment

Shouldn't matter, because we are not using moments by default yet

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop these 3 as well

Co-authored-by: Ricardo Vieira <[email protected]>
@ricardoV94 ricardoV94 merged commit e257fe0 into pymc-devs:main Nov 16, 2021
@ricardoV94
Copy link
Member

Thanks!

morganstrom pushed a commit to morganstrom/pymc that referenced this pull request Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants