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

linalg: Matrix Inverse #828

Merged
merged 34 commits into from
Jul 8, 2024
Merged

linalg: Matrix Inverse #828

merged 34 commits into from
Jul 8, 2024

Conversation

perazz
Copy link
Contributor

@perazz perazz commented May 23, 2024

Compute the multiplicative inverse of a real or complex square matrix: $A \cdot A^{-1} = A^{-1} \cdot A = I_n$ .
Based on LAPACK General factorization (*GETRF) and inversion (*GETRI).

  • base implementation
  • tests
  • exclude unsupported xdp
  • documentation
  • submodule
  • examples
  • subroutine interface

Prior art

  • Numpy: B = inv(A)
  • Scipy: inv(A, overwrite_a=False, check_finite=True)
  • IMSL: .i.A

Proposed implementation

  • B = inv(A [, err]) function interface
  • call invert(A [, pivot] [, err]) in-place (no-allocation) subroutine interface (optional pivot array)
  • .inv.A operator interface

cc: @jalvesz @jvdp1 @loiseaujc @fortran-lang/stdlib

@perazz perazz marked this pull request as ready for review May 23, 2024 15:50
@loiseaujc
Copy link

As far as this is my first review for stdlib, it looks pretty good to me. I was jut wondering about the tests. Wouldn't including a test with a known singular matrix be useful to make sure the whole error handling is working correctly?

@perazz
Copy link
Contributor Author

perazz commented May 27, 2024

Great idea @loiseaujc, thanks!

  • test for singular matrix returning LINALG_ERROR
  • noticed that complex tests were not running -> add them

e10e4c4

@perazz
Copy link
Contributor Author

perazz commented Jun 4, 2024

From Fortran Monthly call:

  • in subroutine invert, add interface to not operate in-place but return inverse to another variable (i.e., call invert(a, am1, [, pivot] [, err])
  • in .inv.A operator, with singular matrix, do not return all zeros, but stop the program

example/linalg/example_inverse3.f90 Outdated Show resolved Hide resolved
example/linalg/example_inverse1.f90 Outdated Show resolved Hide resolved
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @perazz . On overall LGTM. Here are some suggestions.

doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
src/stdlib_linalg_inverse.fypp Show resolved Hide resolved
test/linalg/test_linalg_inverse.fypp Show resolved Hide resolved
src/stdlib_linalg_inverse.fypp Outdated Show resolved Hide resolved
perazz and others added 8 commits June 21, 2024 14:35
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@perazz
Copy link
Contributor Author

perazz commented Jun 21, 2024

Thanks a lot @loiseaujc @jvdp1 @jalvesz for the reviews - I believe I've addressed all your comments (see linked commits in each comment). In d0af9be I've introduced a tiny behavior change:

On exceptions raised when the operator interface (.inv.A) is used, instead of error stopping the program, I think it is more reasonable to instead return an array of NaNs (which are initialized from ieee_value(0.0,ieee_quiet_nan). The reason I think this is better is:

  • operators can be chained, I don't think we want the program to stop in between. For that, there are the subroutine and function interfaces
  • other Fortran operators (+, -, but also matmul etc. ) would behave exactly in the same way: when something goes wrong, they typically return NaNs.

So please do let me know if you also like this update.

doc/specs/stdlib_linalg.md Outdated Show resolved Hide resolved
@jalvesz
Copy link
Contributor

jalvesz commented Jun 27, 2024

Given your idea to enable the operator interface to run through the computation without stopping in case of singular matrices I just suggested a small addition to the documentation. This change seems reasonable to me as even a scalar division would not stop the program but also simply return a NaN.
Otherwise LGTM!!

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Thank you @perazz . LGTM. I think it could be merged.

src/stdlib_linalg.fypp Outdated Show resolved Hide resolved
src/stdlib_linalg.fypp Outdated Show resolved Hide resolved
@perazz
Copy link
Contributor Author

perazz commented Jul 6, 2024

Ok @jvdp1 @jalvesz I've restored the xdp precision. Please let me know if you have further comments. This is probably ready to be merged sometime next week.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @perazz

@perazz
Copy link
Contributor Author

perazz commented Jul 8, 2024

Thank you all, I will merge this now.

@perazz perazz merged commit c8fa301 into fortran-lang:master Jul 8, 2024
17 checks passed
@perazz perazz deleted the matrix_inverse branch July 8, 2024 06:55
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.

None yet

4 participants