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.

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

Low Rank dispatch rules #57

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

Fr0do
Copy link
Contributor

@Fr0do Fr0do commented Sep 11, 2023

Low-rank dispatch rules for Woodbury Identity case of inv and computationally effective trace of low-rank product, resolves #48

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.33% 🎉

Comparison is base (f40c7e1) 78.44% compared to head (0ae5c03) 78.78%.

❗ Current head 0ae5c03 differs from pull request most recent head 190f9f4. Consider uploading reports for the commit 190f9f4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
+ Coverage   78.44%   78.78%   +0.33%     
==========================================
  Files          36       36              
  Lines        2974     2988      +14     
==========================================
+ Hits         2333     2354      +21     
+ Misses        641      634       -7     
Files Changed Coverage Δ
cola/linalg/diag_trace.py 85.71% <100.00%> (+0.78%) ⬆️
cola/linalg/inv.py 90.67% <100.00%> (+0.76%) ⬆️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mfinzi mfinzi left a comment

Choose a reason for hiding this comment

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

Great work @Fr0do 🙏, this is a nice little addition showcasing some of the more complicated dispatch rules 🚀

Let's see if we can generalize this even further with some additional types (U,V need not be dense, Diagonal can be made more general). If you don't get to it, that's fine too, I can finish up with some additional cases later this week

U, V = A.Ms[0].Ms
D_inv = inv(A.Ms[1], **kwargs)
I = Identity(shape=(V.shape[0], U.shape[1]), dtype=V.dtype)
return D_inv - D_inv @ U @ inv(V @ D_inv @ U + I, **kwargs) @ V @ D_inv
Copy link
Collaborator

Choose a reason for hiding this comment

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

very clean!



@dispatch
def inv(A: Sum[Product[Dense,Dense], Diagonal], **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be able to generalize this to
Sum[Product[LinearOperator,LinearOperator] right?

Also I believe we should be able to generalize Diagonal to efficiently_invertible = Union[Diagonal, Identity, ScalarMul], we could extend that further to Triangular, Permutation.

@mfinzi
Copy link
Collaborator

mfinzi commented Sep 12, 2023

I didn't realize you're on a fork, I made some edits and here is a diff:
diff_output.txt

Though the tests pass (with the adjustment to take into account that fixed_normal_samples -> randn in a recent patch)
it seems that the Woodbury dispatch rules are not being hit. This isn't due to any incorrect usage or mistake on your end, but it looks to be a limitation (bug?) in plum-dispatch for parametric types. issubclass(A, Sum[TypeB, TypeC]) will evaluate to true while not matching the dispatch rule for A. I will investigate if this is easily solvable in our cola-plum-dispatch fork that we use.

@Fr0do
Copy link
Contributor Author

Fr0do commented Sep 15, 2023

I didn't realize you're on a fork, I made some edits and here is a diff: diff_output.txt

Is there a way to work collaboratively? I can add mainainers of CoLA to my fork as collaborators. Or maybe there is a way I can create a branch in your repo?

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.

[Feature Request] Low Rank Dispatch rules (Woodbury identity, Trace rules, etc)
2 participants