-
Notifications
You must be signed in to change notification settings - Fork 131
Refactor nlinalg.norm
to match np.linalg.norm
signature and functionaly
#588
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
- Coverage 80.83% 80.83% -0.01%
==========================================
Files 162 162
Lines 46743 46797 +54
Branches 11417 11434 +17
==========================================
+ Hits 37786 37827 +41
- Misses 6714 6722 +8
- Partials 2243 2248 +5
|
I ended up just manually handing the broadcasting cases. Curious if my original approach could work somehow, because the solution I converged on duplicates a lot of the logic handled automatically by Blockwise. I also had to make SVD Blockwise en passant, so that's a nice bonus from this PR. |
8f44bfe
to
d8f0e32
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.
These changes look amazing!
Left some comments, but I don't see anything that should be done structurally differently
Expand TestNorm test coverage
7e580b2
to
2812aca
Compare
I reorganized this PR into two separate commits, one for SVD and one for norm. It should be good to go now. |
Did we have any rewrites targeting SVD Ops? Now they we Blockwise they would need to be tweaked, but since tests pass I assume we don't have any |
No, it was kind of an orphan Op, with no rewrites or gradients. |
Description
Currently,
pt.linalg.norm
does not match the signature ofnp.linalg.norm
-- it is missing theaxis
andkeepdims
arguments, and has no logic for handling complex-valued matrices. This PR fixes this by copy-pasting thenp.linalg.norm
code to pytensor.In addition, some extra logic is needed to allow for tensor-valued inputs to
norm
. This part is still a WIP. The tests need to be refactored to test all the new combinations of keyword arguments as well.This is a preliminary step to addressing pymc-devs/pymc#7101, because the correct transformation between unconstrained lower-triangular matrices to cholesky factorized matrices involves row-normalizing the unconstrained lower-triangular matrix; see here.
Related Issue
Checklist
Type of change