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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix MatrixNormal.random #4368

Merged
merged 6 commits into from Dec 30, 2020
Merged

Fix MatrixNormal.random #4368

merged 6 commits into from Dec 30, 2020

Conversation

Sayam753
Copy link
Member

@Sayam753 Sayam753 commented Dec 22, 2020 •
edited by twiecki

Thank your for opening a PR!

Depending on what your PR does, here are a few things you might want to address in the description:

  • what are the (breaking) changes that this PR makes?
    Fixes Bug in MatrixNormal.sample聽#3585
  • important background, or details about the implementation
    These random methods are amazing 馃ぉ . Just broadcast everything before hand and then go for any calculations.
  • are the changes鈥攅specially new features鈥攃overed by tests and docstrings?
    Yes. I have added a related test.
  • right before it's ready to merge, mention the PR in the RELEASE-NOTES.md

@codecov
Copy link

codecov bot commented Dec 22, 2020 •
edited

Codecov Report

Merging #4368 (4a5ba0f) into master (0402aab) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4368      +/-   ##
==========================================
+ Coverage   87.95%   88.07%   +0.11%     
==========================================
  Files          88       88              
  Lines       14493    14476      -17     
==========================================
+ Hits        12748    12750       +2     
+ Misses       1745     1726      -19
Impacted Files Coverage 螖
pymc3/distributions/multivariate.py 83.64% <100.00%> (+0.72%) 猬嗭笍
pymc3/model.py 89.29% <0.00%> (+0.01%) 猬嗭笍
pymc3/distributions/dist_math.py 92.17% <0.00%> (+0.20%) 猬嗭笍
pymc3/distributions/mixture.py 88.96% <0.00%> (+0.26%) 猬嗭笍
pymc3/distributions/timeseries.py 86.70% <0.00%> (+0.99%) 猬嗭笍
pymc3/distributions/continuous.py 94.37% <0.00%> (+1.06%) 猬嗭笍

MarcoGorelli
MarcoGorelli suggested changes Dec 22, 2020
View reviewed changes
Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Wow, much simpler like this!

pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved
pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/distributions/multivariate.py Outdated Show resolved Hide resolved
pymc3/tests/test_distributions_random.py Outdated Show resolved Hide resolved
@Sayam753
Copy link
Member Author

I addressed all the suggestions above. The PR is ready for another round of review.

@MarcoGorelli MarcoGorelli self-requested a review December 25, 2020 10:59
@michaelosthege
Copy link
Member

The new code looks much better! I can't comment on the math behind these changes, as I'm not familiar enough with it.
I think you can already add the line for the release notes while waiting for @twiecki or @lucianopaz to review the math.

@michaelosthege michaelosthege added this to the vNext (3.11.0) milestone Dec 30, 2020
@Sayam753
Copy link
Member Author

Sayam753 commented Dec 30, 2020 •
edited

Does this PR branch need a rebase with current master?

@twiecki
Copy link
Member

twiecki commented Dec 30, 2020

No, just release notes.

@twiecki twiecki merged commit 91993d8 into pymc-devs:master Dec 30, 2020
@twiecki
Copy link
Member

twiecki commented Dec 30, 2020

Thanks @Sayam753!

@Sayam753 Sayam753 mentioned this pull request Dec 31, 2020
Merged
@Sayam753 Sayam753 mentioned this pull request Jan 14, 2021
Merged
@Sayam753 Sayam753 deleted the fix_matrixnormal branch February 2, 2021 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twiecki twiecki twiecki approved these changes

@lucianopaz lucianopaz Awaiting requested review from lucianopaz

@MarcoGorelli MarcoGorelli Awaiting requested review from MarcoGorelli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.

Bug in MatrixNormal.sample
4 participants
@Sayam753 @michaelosthege @twiecki @MarcoGorelli