Fix error in docstring example for MatrixNormal #7599

roesta07 · 2024-12-01T17:09:11Z

…rrent version

Description

The compute_corr parameter in pm.LKJCholeskyCov is set to True by default, which causes the issue in latest pymc versions.
Also discussed here: https://discourse.pymc.io/t/matrixnormal-example-in-documentation-throws-an-error/16171

Related Issue

Closes DOC: <MatrixNormal Example in Documentation not updated as per latest version ' prefix> #7598

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Type of change

New feature / enhancement
Bug fix
[ x] Documentation
Maintenance
Other (please specify):

📚 Documentation preview 📚: https://pymc--7599.org.readthedocs.build/en/7599/

welcome · 2024-12-01T17:09:13Z

]
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

roesta07 · 2024-12-03T09:43:01Z

Hi @cluhmann as per our conversation here. Also I am not sure why my pre-commit is failing because of different file which i have not touched.

ricardoV94 · 2024-12-03T10:52:52Z

pymc/distributions/multivariate.py

+            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
+                                            sd_dist=sd_dist)


A bit more succinct?

Suggested change

colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,

sd_dist=sd_dist)

colchol_packed, _, _ = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2, sd_dist=sd_dist)

@ricardoV94 Yeah also if we are using this approach then there is No need for expand_packed_triangular(line:1832) since it's already expanded. Also I will rename "cholchol_packed" to "cholchol"

ricardoV94 · 2024-12-03T10:53:22Z

pymc/distributions/multivariate.py

-            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,
-                                               sd_dist=sd_dist)
+            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
+                                            sd_dist=sd_dist)
            colchol = pm.expand_packed_triangular(3, colchol_packed)

            # Setup left covariance matrix
            scale = pm.LogNormal('scale', mu=np.log(true_scale), sigma=0.5)
            rowcov = pt.diag([scale**(2*i) for i in range(m)])

            vals = pm.MatrixNormal('vals', mu=mu, colchol=colchol, rowcov=rowcov,


The suggestion can't be applied directyl, but this seems more clean

Suggested change

vals = pm.MatrixNormal('vals', mu=mu, colchol=colchol, rowcov=rowcov,

vals = pm.MatrixNormal('vals', mu=mu, colchol=colchol, rowcov=rowcov, observed=data)

Missid this. I would put in same line, looks odd split

ricardoV94 · 2024-12-03T10:54:30Z

@roesta07 you can ignore that pre-commit fail, it's indeed not related to your PR

roesta07 · 2024-12-04T06:19:18Z

pymc/distributions/multivariate.py

@@ -1827,16 +1827,16 @@ class MatrixNormal(Continuous):
        with pm.Model() as model:
            # Setup right cholesky matrix
            sd_dist = pm.HalfCauchy.dist(beta=2.5, shape=3)
-            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,
-                                               sd_dist=sd_dist)
+            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,


Suggested change

colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,

colchol, _, _ = pm.LKJCholeskyCov('colchol', n=3, eta=2,

roesta07 · 2024-12-04T06:19:21Z

pymc/distributions/multivariate.py

+            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
+                                            sd_dist=sd_dist)


@ricardoV94 Yeah also if we are using this approach then there is No need for expand_packed_triangular(line:1832) since it's already expanded. Also I will rename "cholchol_packed" to "cholchol"

roesta07 · 2024-12-04T14:40:39Z

pymc/distributions/multivariate.py

-            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,
-                                               sd_dist=sd_dist)
+            colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
+                                            sd_dist=sd_dist)
            colchol = pm.expand_packed_triangular(3, colchol_packed)


Suggested change

colchol = pm.expand_packed_triangular(3, colchol_packed)

roesta07 · 2024-12-07T10:20:35Z

Hi @ricardoV94, I have fixed the docs string as a fresh commit.
Aside this, there is a different part (below) from the same documentation file is without the model context, should I add with pm.Model() as model: on top of this? of leave it as it is

colcov = np.array([[1.0, 0.5], [0.5, 2]])
rowcov = np.array([[1, 0, 0], [0, 4, 0], [0, 0, 16]])
m = rowcov.shape[0]
n = colcov.shape[0]
mu = np.zeros((m, n))
vals = pm.MatrixNormal("vals", mu=mu, colcov=colcov, rowcov=rowcov)

ricardoV94 · 2024-12-10T09:08:40Z

@roesta07 Ideally the code snippet would be completely correct that you could copy past and run in the terminal, so it would be better to put it in a model context

…rrent version

Co-authored-by: Ricardo Vieira <[email protected]>

roesta07 · 2024-12-10T12:13:36Z

@ricardoV94 I have added the model context at the top along with the previous changes.

ricardoV94 · 2024-12-10T13:25:04Z

pymc/distributions/multivariate.py

@@ -1792,7 +1792,7 @@ class MatrixNormal(Continuous):
    --------
    Define a matrixvariate normal variable for given row and column covariance
    matrices::
-
+    with pm.Model() as model:


Should add a .. code:: python with white line above and below to render nicely

Also include imports so example is fully specified

added imports on top.

added codeblocks .. code:: python with appropriate whitespaces
Check these recent changes

welcome · 2024-12-11T09:07:13Z

]
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

ricardoV94 · 2024-12-11T09:07:15Z

Thanks @roesta07

codecov · 2024-12-11T09:38:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.83%. Comparing base (a714b24) to head (c2edfb1).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7599      +/-   ##
==========================================
- Coverage   92.84%   92.83%   -0.02%     
==========================================
  Files         106      106              
  Lines       17745    17748       +3     
==========================================
+ Hits        16476    16477       +1     
- Misses       1269     1271       +2

Files with missing lines	Coverage Δ
pymc/distributions/multivariate.py	`93.58% <ø> (ø)`

... and 1 file with indirect coverage changes

Co-authored-by: Rojan Shrestha <[email protected]> Co-authored-by: Ricardo Vieira <[email protected]>

ricardoV94 reviewed Dec 3, 2024

View reviewed changes

ricardoV94 added the docs label Dec 3, 2024

ricardoV94 changed the title ~~updated docstring for pm.MatrixNormal to ensure compatibility with cu…~~ Fix error in docstring example for MatrixNormal Dec 3, 2024

roesta07 commented Dec 4, 2024

View reviewed changes

Rojan Shrestha and others added 4 commits December 10, 2024 15:17

updated docstring for pm.MatrixNormal to ensure compatibility with cu…

5155844

…rrent version

Update pymc/distributions/multivariate.py

3f57642

Co-authored-by: Ricardo Vieira <[email protected]>

updated docstring for pm.MatrixNormal after review

c219a4a

updated docstring for pm.MatrixNormal-after review

7e47f7a

roesta07 force-pushed the docstring_update branch from fa74e40 to 7e47f7a Compare December 10, 2024 12:03

ricardoV94 reviewed Dec 10, 2024

View reviewed changes

after adding codeblocks,imports

c2edfb1

ricardoV94 approved these changes Dec 11, 2024

View reviewed changes

ricardoV94 merged commit 5d51953 into pymc-devs:main Dec 11, 2024
5 checks passed

fonnesbeck pushed a commit to fonnesbeck/pymc that referenced this pull request Feb 1, 2025

Fix error in docstring example for MatrixNormal (pymc-devs#7599)

e6ca7e7

Co-authored-by: Rojan Shrestha <[email protected]> Co-authored-by: Ricardo Vieira <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error in docstring example for MatrixNormal #7599

Fix error in docstring example for MatrixNormal #7599

roesta07 commented Dec 1, 2024 •

edited by ricardoV94

Loading

welcome bot commented Dec 1, 2024

roesta07 commented Dec 3, 2024

ricardoV94 Dec 3, 2024

roesta07 Dec 4, 2024

ricardoV94 Dec 3, 2024 •

edited

Loading

ricardoV94 Dec 10, 2024

ricardoV94 commented Dec 3, 2024

roesta07 Dec 4, 2024

roesta07 Dec 4, 2024

roesta07 Dec 4, 2024

roesta07 commented Dec 7, 2024 •

edited

Loading

ricardoV94 commented Dec 10, 2024

roesta07 commented Dec 10, 2024

ricardoV94 Dec 10, 2024 •

edited

Loading

ricardoV94 Dec 10, 2024

roesta07 Dec 10, 2024 •

edited

Loading

welcome bot commented Dec 11, 2024

ricardoV94 commented Dec 11, 2024

codecov bot commented Dec 11, 2024

		colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
		sd_dist=sd_dist)

	colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
	sd_dist=sd_dist)
	colchol_packed, _, _ = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2, sd_dist=sd_dist)

	vals = pm.MatrixNormal('vals', mu=mu, colchol=colchol, rowcov=rowcov,
	vals = pm.MatrixNormal('vals', mu=mu, colchol=colchol, rowcov=rowcov, observed=data)

	colchol_packed = pm.LKJCholeskyCov('colcholpacked', n=3, eta=2,compute_corr=False,
	colchol, _, _ = pm.LKJCholeskyCov('colchol', n=3, eta=2,

Fix error in docstring example for MatrixNormal #7599

Fix error in docstring example for MatrixNormal #7599

Conversation

roesta07 commented Dec 1, 2024 • edited by ricardoV94 Loading

Description

Related Issue

Checklist

Type of change

welcome bot commented Dec 1, 2024

roesta07 commented Dec 3, 2024

ricardoV94 Dec 3, 2024

Choose a reason for hiding this comment

roesta07 Dec 4, 2024

Choose a reason for hiding this comment

ricardoV94 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

ricardoV94 Dec 10, 2024

Choose a reason for hiding this comment

ricardoV94 commented Dec 3, 2024

roesta07 Dec 4, 2024

Choose a reason for hiding this comment

roesta07 Dec 4, 2024

Choose a reason for hiding this comment

roesta07 Dec 4, 2024

Choose a reason for hiding this comment

roesta07 commented Dec 7, 2024 • edited Loading

ricardoV94 commented Dec 10, 2024

roesta07 commented Dec 10, 2024

ricardoV94 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

ricardoV94 Dec 10, 2024

Choose a reason for hiding this comment

roesta07 Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

welcome bot commented Dec 11, 2024

ricardoV94 commented Dec 11, 2024

codecov bot commented Dec 11, 2024

Codecov Report

roesta07 commented Dec 1, 2024 •

edited by ricardoV94

Loading

ricardoV94 Dec 3, 2024 •

edited

Loading

roesta07 commented Dec 7, 2024 •

edited

Loading

ricardoV94 Dec 10, 2024 •

edited

Loading

roesta07 Dec 10, 2024 •

edited

Loading