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: Singular Value Decomposition #808

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

perazz
Copy link
Contributor

@perazz perazz commented May 9, 2024

Singular Value Decomposition

of $A = U \cdot S \cdot V^T$, for real and complex matrices, based on LAPACK *GESDD functions.

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

Prior art

  • NumPy: svd(a, full_matrices=True, compute_uv=True, hermitian=False)
  • Scipy: svd(a, full_matrices=True, compute_uv=True, overwrite_a=False, check_finite=True, lapack_driver='gesdd')
  • Scipy: svdvals(a, overwrite_a=False, check_finite=True)

Proposed implementation

  • Subroutine interface
    call svd(a,s [,u,vt,overwrite_a,full_matrices,err])

  • Function interface
    s = svdvals(a [, err])

GESDD allows to overwrite a with either u or v vectors, saving storage and matrix copies.
The internal GESDD task is chosen based on user inputs in order to minimize the number of allocations in particular:

  • Whether u/v are provided, and if a can be overwritten
  • Whether storage is for full matrices or partial matrices (min(m,n))

cc: @jvdp1 @jalvesz @zoziha: I believe this is ready for consideration. I'm investigating what is causing ifx to segfault, although I don't have a local installation (on Apple silicon) EDIT: segfault fixed (ifx compiler bug)

@perazz
Copy link
Contributor Author

perazz commented May 9, 2024

ifx has an internal compiler error on the test: will need to investigate

@perazz perazz marked this pull request as ready for review May 9, 2024 09:57
@perazz
Copy link
Contributor Author

perazz commented May 9, 2024

Fixed:

@jalvesz
Copy link
Contributor

jalvesz commented May 25, 2024

@perazz I just started to read the PR, brilliant! I would like to bring an idea for discussion regarding the naming for optional variables and their internal counterparts. I've seen elsewhere and started to stick to the following naming style:

subroutine my_sub( a, some_optional )
 <type>, intent(<>) :: a
 <type>, intent(in), optional :: some_optional
 <type>                 :: some_optional_ !> same name as the user-exposed name with the "_" suffix
end subroutine

I found this useful for two reasons (1) less head-scratching to find a name for the corresponding internal counterpart (2) easy to find the variable or change their names if needed.

Following the use of optionals, I also found that is easier to read the implementations using the following assignment pattern (say for a logical):

some_optional_ = .false.
if(present(some_optional)) some_optional_ = some_optional

Which I feel, makes it clear what is the default at a glance when reading the code.

I've being thinking about the overwrite_a boolean flag. This design currently implies:

  • If .false., the matrix is not touched, but an internal allocation is incurred.
  • If .true., no internal allocation is incurred, but the matrix is lost as it is recycled as a temp buffer.

What if instead of managing this choice in this manner, an optional temp array is allowed? such that:

  • if(present(temp)) the matrix a is untouched, temp is used (and recycled) for internal processing.
  • else internal allocation is done, a remains untouched.

@perazz
Copy link
Contributor Author

perazz commented May 26, 2024

Appreciate your reviewing @jalvesz.
Though I'had stuck to the similar logic overwrite_a -> copy_a of the other linear algebra routines, I agree it's not super clear. So I am fixing it here and will update the other instances to have the same new approach.

Regarding optional present arguments, things were a bit tricky in the SVD because the LAPACK backend allows to return this data on top of A. So if A can be destroyed (i.e. it is an internal copy, or, the user said it can be destroyed), and U or V is not requested on output (not present) but must be computed anyways (the other matrix was requested), we don't want to allocate another temporary for it. We can just spare this additional allocation by pointing the routine to replace it on top of A.

So yes, it is a bit hard to read in the code, because the backend logic is also pretty cumbersome. But ultimately, the goal here was to strive for performance i.e. to avoid allocations wherever possible

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 just have a few minor comments.

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
example/linalg/example_svd1.f90 Outdated Show resolved Hide resolved
example/linalg/example_svd1.f90 Outdated Show resolved Hide resolved
example/linalg/CMakeLists.txt 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
perazz and others added 4 commits May 28, 2024 21:55
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 May 28, 2024

Thank you @jvdp1 @jalvesz for your reviews, it looks much better now!

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

3 participants