-
Notifications
You must be signed in to change notification settings - Fork 160
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: Eigenvalues and Eigenvectors #816
base: master
Are you sure you want to change the base?
Conversation
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. Here are some comments regarding the specs. I'll focus on the code later
From Fortran Monthly call:
For example, NumPy returns if not isComplexType(t) and all(w.imag == 0.0):
w = w.real
vt = vt.real
result_t = _realType(result_t)
else:
result_t = _complexType(result_t) Thank you @jvdp1 for the reviews. As pointed out during our last Monthly call with @everythingfunctional @jalvesz, I've added the option to return the stdlib/src/stdlib_linalg_eigenvalues.fypp Lines 346 to 350 in e6aa0f5
|
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>
if (present(upper)) then | ||
symmetric_triangle_task = merge('U','L',upper) | ||
else | ||
symmetric_triangle_task = 'L' | ||
endif |
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.
Would suggest to put the default value explicitly outside the conditional evaluation for optionals.
if (present(upper)) then | |
symmetric_triangle_task = merge('U','L',upper) | |
else | |
symmetric_triangle_task = 'L' | |
endif | |
symmetric_triangle_task = 'L' | |
if (present(upper)) symmetric_triangle_task = merge('U','L',upper) |
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.
I also prefer @jalvesz 's notation.
neig = size(lambda,kind=ilp) | ||
lda = m | ||
|
||
if (.not.(k>0 .and. m==n)) then |
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.
I think this conditional can be represented this way:
if (.not.(k>0 .and. m==n)) then | |
if (k<=0 .or. m/=n) then |
'invalid or matrix size a=',[m,n],', must be square.') | ||
call linalg_error_handling(err0,err) | ||
return | ||
elseif (.not.neig>=k) then |
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.
Why not simply?:
elseif (.not.neig>=k) then | |
elseif (neig<k) then |
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.
My coding practice is that checking for .not.verified
is always better because it includes the edge cases (e.g. what if neig
is NaN
because we provided an unallocated array?
But, we're talking about tiny details, so I'm also ok with checking if (verified)
!
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.
If what is written here https://fortran-lang.discourse.group/t/how-to-fill-an-integer-array-with-some-nans/3696 is correct, an integer cannot be NaN
. What might cause issues is that this query neig = size(lambda,kind=ilp)
can return 1
with some compilers if the allocatable has not been allocated. To be in the safe side I would actually change in line 180 for
neig = 0
if(allocated(lambda)) neig = size(lambda,kind=ilp)
if (present(overwrite_a)) then | ||
copy_a = .not.overwrite_a | ||
else | ||
copy_a = .true._lk | ||
endif |
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.
if (present(overwrite_a)) then | |
copy_a = .not.overwrite_a | |
else | |
copy_a = .true._lk | |
endif | |
copy_a = .true._lk | |
if (present(overwrite_a)) copy_a = .not.overwrite_a |
k = min(m,n) | ||
neig = size(lambda,kind=ilp) | ||
|
||
if (.not.(k>0 .and. m==n)) then |
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.
if (.not.(k>0 .and. m==n)) then | |
if (k<=0 .or. m/=n) then |
', must be square.') | ||
call linalg_error_handling(err0,err) | ||
return | ||
elseif (.not.neig>=k) then |
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.
elseif (.not.neig>=k) then | |
elseif (neig<k) then |
if (present(vectors)) then | ||
! No need to copy A anyways | ||
copy_a = .false. | ||
elseif (present(overwrite_a)) then | ||
copy_a = .not.overwrite_a | ||
else | ||
copy_a = .true._lk | ||
endif |
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.
if (present(vectors)) then | |
! No need to copy A anyways | |
copy_a = .false. | |
elseif (present(overwrite_a)) then | |
copy_a = .not.overwrite_a | |
else | |
copy_a = .true._lk | |
endif | |
copy_a = .true._lk | |
if (present(vectors)) then | |
! No need to copy A anyways | |
copy_a = .false. | |
elseif (present(overwrite_a)) then | |
copy_a = .not.overwrite_a | |
endif |
Computing eigenvalues and eigenvectors of a square matrix:$A \cdot \bar{v} - \lambda \cdot \bar{v} = 0$ .
Based on LAPACK General (
*GEEV
) or real symmetric (*SYEV
) / complex Hermitian (*HEEV
) operations.xdp
subroutine
interfaceGeneral matrix
Prior art
eigenvalues, eigenvectors = eig(a)
eigenvalues = eigvals(a)
eig(a, b=None, left=False, right=True, overwrite_a=False, overwrite_b=False, check_finite=True, homogeneous_eigvals=False)
Proposed implementation
Eigenvalues only: two function interfaces
lambda = eigvals(A [, err])
Eigendecomposition: subroutine interface
call eig(a,lambda [,right] [,left] [,overwrite_a] [,err])
Eigenvalues of real matrices can be complex-conjugate. To avoid confusion,
right
,left
are postprocessed and always returned ascomplex
arrays. LAPACK instead stores(re,im)
of conjugates as consecutive real values[j, j+1]
in a real array, this would be very confusing and hard to track down.The eigendecomposition can be used either for eigenvalues only, or to retrieve left or right eigenvectors.
Real Symmetric / Complex Hermitian
Prior art
eigenvalues, eigenvectors = eigh(a, uplo='L')
eigenvalues = eigvalsh(a)
eigh(a, b=None, *, lower=True, eigvals_only=False, overwrite_a=False, overwrite_b=False, turbo=<object object>, eigvals=<object object>, type=1, check_finite=True, subset_by_index=None, subset_by_value=None, driver=None)
Proposed implementation
Eigenvalues only: two function interfaces
lambda = eigvalsh(A [, err])
Eigendecomposition: subroutine interface
call eigh(a,lambda [,vectors] [,upper_a] [,overwrite_a] [,err] )
vectors
that can be used as temporary storage fora
.Note: the LAPACK backends are not
pure
due to functions with intent(out) arguments. The current proposed interfaces are ready to be made pure (e.g., function interface with allintent(in)
arguments) to make it easy when the backend will be pure.I believe this is ready for consideration, thank you @jvdp1 @jalvesz @fortran-lang/stdlib