-
Notifications
You must be signed in to change notification settings - Fork 68
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
Custom rule in SparseMatrixCSC #2013
Comments
Hi, I tested more on the sparse matrix. I bumped into more issues. In the following MWE, I tried to get the autodiff from an inversion matrix: y = x⁻¹, where the element x[3,2] is changed by the parameter r (i.e. x[3,2] = x[3,2] + r):
Here I compute the pullback by writing
I expect the pullback dx[3,2] = dr in this simple case. However, if I just wrote the custom rule for the only inversion part, then dx[3,2] != dr. After testing, this issue also only happened in the sparse matrices, any thought and insight would be appreciated. Thanks! |
CC DARPA-ASKEM/sciml-service#182 SciML/SciMLSensitivity.jl#1139 it seems a lot of things hit this. @wsmoses is a custom rule the right direction here? |
cc @ptiede who was looking at the sparse rules recently |
OK so the SciML issue is just a duplicate of #1970 The reason for the custom rule, @ChrisRackauckas, was that Enzyme's native gradient was slow for the sparse matmul #1682, so I implemented a custom rule. The problem with the rule I defined is that it explicitly does not handle the case where the sparse matrix is duplicated. This is entirely because I lacked the cycles to implement this properly since it requires something like dx .+= dy * (B.val)' does not preserve the correct sparsity pattern for I had initially hoped that since the internal rule was written for when the sparse matrix activity is This is why @hochunlin rules are incorrect; they need to preserve the matrix structure. For sparse matrices this is subtle. Specifically, the returned gradient looks weird because Enzyme is now pointing to different memory since I'll add a Duplicated |
That's fine. I think the key for us is that this is likely the last thing blocking getting a full end-to-end application in PDEs working (I think), so a fallback to just differentiate the code directly is actually what I was hoping someone could come up with, at least to see if the full applications are working.
Yeah if you couldn't tell I'm not a fan of this behavior. |
Ok PR #2109 should fix the SciML issue @ChrisRackauckas. @hochunlin, does the above discussion clarify why the custom rules you wrote aren't correct? The main thing is that broadcasting does not preserve the structural zeros. On the brightside, PR #2109 makes it so that your matmul rule likely isn't needed anymore. For dx .+= - inv(Matrix(x.val))' * dy * inv(Matrix(x.val))' will need to change. |
@ptiede Yes. It is clear that broadcasting does not preserve the structural zeros, and it causes the issue. Then how should I change the custom rule |
I am writing some reverse-mode custom rules for manipulating sparse matrices with Enzyme. However, I did not get the correct result compared to the finite-difference result. The following is the MWE:
In this example, I tried to get the autodiff from a matrix multiplication: S = A*B, where B is a constant matrix.
I test the four cases:
Case 1. Dense matrix multiplication with internal enzyme rule
Case 2. Dense matrix multiplication with my custom enzyme rule
Case 3. Sparse matrix multiplication with internal enzyme rule
Case 4. Sparse matrix multiplication with my custom enzyme rule
The case 4 does not give the right result. The custom rule in sparse matrix multiplication does not work because the pullbacks passed to it are incorrect, such as passing dA[2,1] wrongly to dA[1,1].
Now, I am confused why case 3 (internal Enzyme rule) can work, but case 4 (custom Enzyme rule) doesn't. Is there a subtlety in the implementation of the custom rule for the sparse matrix I am missing? Thanks for any suggestion or insight in advance!
The text was updated successfully, but these errors were encountered: