Skip to content

Fix README rand_sparse_tri QuickStart example (#92) - #93

Merged
theo-barfoot merged 4 commits into
cai4cai:mainfrom
NusaybahA:fix-92-readme-quickstart
Jul 29, 2026
Merged

Fix README rand_sparse_tri QuickStart example (#92) #93
theo-barfoot merged 4 commits into
cai4cai:mainfrom
NusaybahA:fix-92-readme-quickstart

Conversation

@NusaybahA

@NusaybahA NusaybahA commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replaced the invalid unit_triangular parameter with strict in the README Quickstart examples to align with the current rand_sparse_tri() API.
  • Updated example usage to reflect the correct supported parameters.
  • Clarified the sparsity comment to accurately describe the generated 1000×1000 dense matrix size and avoid confusion around event_size=1000.

Testing

  • Verified that the README examples now use the correct strict keyword.
  • Confirmed the sparsity comment accurately reflects the matrix dimensions and sparsity ratio.

@NusaybahA
NusaybahA marked this pull request as draft July 28, 2026 11:35
@NusaybahA
NusaybahA force-pushed the fix-92-readme-quickstart branch from 3daf005 to 68968df Compare July 28, 2026 12:33
@NusaybahA
NusaybahA marked this pull request as ready for review July 28, 2026 13:39
@aymuos15

aymuos15 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Congrats on your first contribution! 🎉

The strict fix is correct, and requires_grad_(True) fixes a second latent bug: without it, the trailing scale_tril.grad.shape line hits AttributeError on None. I ran the snippet end-to-end and it works now.

Maybe another small change - Three comments still say "unit triangular," but strict=True gives a zero diagonal, not a unit one.

-# Create sparse lower triangular matrix (unit triangular, no diagonal)
+# Create sparse lower triangular matrix (strictly lower triangular, no diagonal)
 dist_ldlt = SparseMultivariateNormal(
     loc=loc,
     diagonal=diagonal,
-    scale_tril=scale_tril  # Unit lower triangular
+    scale_tril=scale_tril  # Strictly lower triangular
 )
 dist_precision = SparseMultivariateNormal(
     loc=loc,
     diagonal=precision_diagonal,
-    precision_tril=precision_tril  # Unit triangular precision factor
+    precision_tril=precision_tril  # Strictly triangular precision factor
 )

@NusaybahA

Copy link
Copy Markdown
Contributor Author

After reviewing the code I updated any remaining comments that included "unit triangular" ensuring they align with the rest of the code and so that the comments are clear to read.

@aymuos15

Copy link
Copy Markdown
Contributor

@theo-barfoot LGTM

@theo-barfoot theo-barfoot changed the title Title: Fix README rand_sparse_tri QuickStart example (#92) Fix README rand_sparse_tri QuickStart example (#92) Jul 29, 2026

@theo-barfoot theo-barfoot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for this, this looks good. The replacement of unit_triangular with strict correctly matches the current rand_sparse_tri API, and the values used for the LDLᵀ and LLᵀ examples are appropriate. Adding requires_grad_(True) also fixes the gradient section at the end of the example.

I found one small pre-existing terminology issue, that was my mistake, which it would be good to correct. In the updated sparsity comment: 5,000 nonzeros in a 1000 × 1000 matrix corresponds to 0.5% density, or 99.5% sparsity.

So I would change the comment to:

nnz=5000, # 5000 nonzeros per 1000x1000 matrix (99.5% sparsity)

Once that wording is corrected, this looks ready to merge.

@NusaybahA

Copy link
Copy Markdown
Contributor Author

I have now corrected the sparsity comment to state a 99.5% sparsity, to make the wording clear.

@aymuos15

Copy link
Copy Markdown
Contributor

Thanks for the review @theo-barfoot !

@theo-barfoot theo-barfoot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! Congrats on your first contribution 😃

@theo-barfoot
theo-barfoot merged commit b96fc72 into cai4cai:main Jul 29, 2026
9 checks passed
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.

3 participants