search

opencollab / arpack-ng

Collection of Fortran77 subroutines designed to solve large scale eigenvalue problems.
Other
290 stars 124 forks source link

Fix pyarpack #432

Closed fghoussen closed 1 year ago

fghoussen commented 1 year ago

Pull request purpose

fixing issue #430

FabienPean commented 1 year ago

I don't think all those pyarpack changes are warranted. By changing all input matrices to an hermitian/symmetric matrix, it reduces the coverage of solvers which handle non-symmetric (LU, QR, etc).

fghoussen commented 1 year ago

I don't think all those pyarpack changes are warranted.

They at least make sense! As you remarked it, hermitian makes better sense for complex.

By changing all input matrices to an hermitian/symmetric matrix, it reduces the coverage of solvers which handle non-symmetric (LU, QR, etc).

The coverage is not that bad (about 70% AFAIR - we had one when CI was done with travisCI). Historically, the coverage is the one of the original arpack repo: no dev from there (arpack-ng is a fork that "only" maintain the original arpack) so coverage has (and will) always be the same. These tests were only added to make sure that stuffs like ICB, arpackSolver.hpp and python binding (based on ICB) were working (not considering coverage) so solving the same case is OK if it helps stability/repeatability.

FabienPean commented 1 year ago

The coverage is not that bad (about 70% AFAIR - we had one when CI was done with travisCI). Historically, the coverage is the one of the original arpack repo: no dev from there (arpack-ng is a fork that "only" maintain the original arpack) so coverage has (and will) always be the same. These tests were only added to make sure that stuffs like ICB, arpackSolver.hpp and python binding (based on ICB) were working (not considering coverage) so solving the same case is OK if it helps stability/repeatability.

True, the complex problem anyway redirect to the same Fortran functions under the hood (cnaupd/cneupd), there is no symmetric variants of those (csaupd/cseupd) so using hermitian is the same as non-hermitian. If arpackmm tests non-symmetric problem and solver, so pyarpack could check whatever.

By browsing cnaupd documentation, I noticed a detail of importance:

This is intended to be used to find a few eigenpairs of a complex linear operator OP with respect to a semi-inner product defined by a hermitian positive semi-definite real matrix B. B may be the identity matrix. NOTE: if both OP and B are real, then ssaupd or snaupd should be used.

It seems to imply that B must be real (no imaginary part) and hermitian (so symmetric?) semi-definite matrix. The current B matrices used for the tests might be not valid ?

fghoussen commented 1 year ago

complex problem anyway redirect to the same Fortran functions under the hood (cnaupd/cneupd), there is no symmetric variants of those (csaupd/cseupd) so using hermitian is the same as non-hermitian

Right!

The current B matrices used for the tests might be not valid ?

Main goal to achieve here is to get stable/repeatable tests (no focus on coverage): is it worth to make B valid/real in #436 to improve test stability?

fghoussen commented 1 year ago

436 fix is prefered.

FabienPean commented 1 year ago

Main goal to achieve here is to get stable/repeatable tests (no focus on coverage): is it worth to make B valid/real in #436 to improve test stability?

Hard to say, I just saw that in the comments for cnaupd, but the exact description (real matrix) is not repeated when describing the modes 2 and 3 below. Besides, the tests with complex values in Fortran (in EXAMPLES/COMPLEX) never involve an imaginary part for the matrices. Since the test was passing already and without clear idea on how to improve, I will avoid fiddling further :)