Skip to content

Conversation

@shahsaurabh0605
Copy link

NMatrix#inverse_exact was not correctly implemented in math.cpp. I solved this problem. Please review this and suggest if there is anything more to add.

@shahsaurabh0605
Copy link
Author

Can you be more specific on how it could be more efficient?

@translunar
Copy link
Member

@shahsaurabh0605 Sure. It seems like you're declaring a lot of temporary variables when you could just be writing directly. They may be optimized out for some dtypes, but I'm not sure if that's the case for others.

@shahsaurabh0605
Copy link
Author

@MohawkJohn See the thing is inverse_exact won't give correct outputs if those temporary variables are not declared. Just using the array, removing temporary variables won't work.

@translunar
Copy link
Member

I don't see why that should be the case. For example, you do this:

DType i1= A[2*lda+2];
DType i2 = i1;
B[1]      = (- A[1]     * i2 + A[2]     * h2) / det;
DType i3=i2;
B[ldb]    = (- d1   * i2 + f2 * g1)   / det; // B = -di + fg
B[ldb+1]  = (  a1     * i3 - c2     * g2)   / det; // E = ai - cg

You're creating a lot of unnecessary copies of i1. Why?

@shahsaurabh0605
Copy link
Author

@MohawkJohn you were correct. I have to use the local variables only once. Made the required changes in the code.

@translunar
Copy link
Member

I'm looking at this and still don't see why you need temporary variables at all.

@shahsaurabh0605
Copy link
Author

Actually why do we need a different array B at all! What if the result is computed itself in array A and the output shown. This could save a lot of space and will be more efficient. I have made the code ready which is using just array A to return the result.

@translunar
Copy link
Member

You need B because the operation is destructive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants