-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
|
Fixed:
|
@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
What if instead of managing this choice in this manner, an optional temp array is allowed? such that:
|
Appreciate your reviewing @jalvesz. Regarding optional 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 |
There was a problem hiding this 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.
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. LGTM. IMO it can be merged
Thank you @jvdp1. I'll wait for a few more days, and if there are no comments, will merge this one. Thank you all for the reviews. |
Singular Value Decomposition
of$A = U \cdot S \cdot V^T$ , for
real
andcomplex
matrices, based on LAPACK*GESDD
functions.xdp
subroutine
interfacePrior art
svd(a, full_matrices=True, compute_uv=True, hermitian=False)
svd(a, full_matrices=True, compute_uv=True, overwrite_a=False, check_finite=True, lapack_driver='gesdd')
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
orv
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:
u
/v
are provided, and ifa
can be overwrittenmin(m,n)
)cc: @jvdp1 @jalvesz @zoziha: I believe this is ready for consideration.
I'm investigating what is causingEDIT: segfault fixed (ifx compiler bug)ifx
to segfault, although I don't have a local installation (on Apple silicon)