-
Notifications
You must be signed in to change notification settings - Fork 121
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
Adds functions to rewrite cholesky decomposition of identity and diagonal matrices #925
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
=======================================
Coverage 81.75% 81.76%
=======================================
Files 183 183
Lines 47763 47796 +33
Branches 11619 11635 +16
=======================================
+ Hits 39050 39081 +31
+ Misses 6523 6522 -1
- Partials 2190 2193 +3
|
487d6de
to
736782b
Compare
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.
Some comments and suggestions. I'd also ask that you add a test to make sure the cases that should not be rewritten are, in fact, not rewritten. For example, pt.linalg.cholesky(pt.eye(7, k=-1) * x)
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.
Looking great, just a couple nitpicks. Could you also add a batch case to each of test_cholesky_diag_from_diag
and test_cholesky_diag_from_eye_mul
, then I think we'll be there.
0b69d85
to
365b6df
Compare
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.
LGTM
90b0e43
to
fd79395
Compare
fd79395
to
cf87362
Compare
…onal matrices (pymc-devs#925) * fixed merge conflicts * fixed failing tests and added rewrite for pt.diag * minor changes; added test to not apply rewrite * added test for batched case and more cases of not applying rewrite * minor changes
@jessegrabowski these PRs were missing labels, specially for the automatic release notes :( |
Description
Add a function to rewrite cholesky(eye) -> eye and cholesky(diag) -> sqrt(diag)
Related Issue
Checklist
Type of change