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

ENH: update to arpack-ng 3.8.0 release #14786

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dimpase
Copy link
Contributor

@dimpase dimpase commented Sep 30, 2021

Important changes are more standard-conforming code.

We ignore their Fortran 90 files for the time being, although they
might allow a simpler interface.

helps with gh-13222

More conformance to Fortran standards

I took the *.f files in https://github.com/opencollab/arpack-ng/tree/3.8.0/SRC and
copied them over the corresponding files in ARPACK module of scipy.

Additional information

one test fails, but it might be merely numerical noise (different scaling of eigenvectors):

scipy/sparse/linalg/eigen/arpack/tests/test_arpack.py:261: in eval_evec
    assert_allclose(LHS, RHS, rtol=rtol, atol=atol, err_msg=err)
E   AssertionError: 
E   Not equal to tolerance rtol=0.00178814, atol=0.000357628
E   error for eigsh:standard, typ=f, which=LA, sigma=0.5, mattype=aslinearoperator, OPpart=None, mode=cayley
E   Mismatched elements: 6 / 12 (50%)
E   Max absolute difference: 0.02621984
E   Max relative difference: 0.00576239
E    x: array([[-3.371152, -0.637804],
E          [-4.576386,  0.270926],
E          [-3.157935,  0.485361],...
E    y: array([[-3.353555, -0.637982],
E          [-4.550167,  0.270658],
E          [-3.140609,  0.485183],...

Edit: the above failure was due to a bug the scipy test code, which I corrected in the 2nd commit.

Important changes are more standard-conforming code.

We ignore their Fortran 90 files for the time being, although they
might allow a simpler interface.
@github-actions github-actions bot added Fortran Items related to the internal Fortran code base scipy.sparse.linalg labels Sep 30, 2021
One tests for correctness eigenpairs, in a loop,
which is meant to allow for a slack in Krylov subspaces method.

One should 1st check whether one got correct eigenvalues, and try again
if they are not the wanted ones.
The current implementation does it backwards: it checks whether
the eigenpair works, without 1st checking for eigenvalues.

This commit corrects this programming error
@dimpase
Copy link
Contributor Author

dimpase commented Sep 30, 2021

The new error is in a different test (coming from the same, changed by the last commit, loop):

scipy/sparse/linalg/eigen/arpack/tests/test_arpack.py:271: in eval_evec
    assert_allclose(LHS, RHS, rtol=rtol, atol=atol, err_msg=err)
E   AssertionError: 
E   Not equal to tolerance rtol=4.44089e-13, atol=4.44089e-13
E   error for eigsh:general, typ=D, which=LM, sigma=None, mattype=csr_matrix, OPpart=None, mode=normal
E   Mismatched elements: 1 / 12 (8.33%)
E   Max absolute difference: 4.38825759e-12
E   Max relative difference: 1.14883516e-12
E    x: array([[  4.684541 +4.460031j,   0.806818 -3.120965j],
E          [  1.891598 +5.961914j,   6.016466-12.95253j ],
E          [  0.573331 +6.620032j, -16.820689 -4.861421j],...
E    y: array([[  4.684541 +4.460031j,   0.806818 -3.120965j],
E          [  1.891598 +5.961914j,   6.016466-12.95253j ],
E          [  0.573331 +6.620032j, -16.820689 -4.861421j],...

this does look like a numerical noise - I don't understand why it was not seen earlier (in fact, the same can be seen on the master branch with only the last commit applied, so that was somehow hidden.

@rgommers
Copy link
Member

rgommers commented Oct 2, 2021

Thanks @dimpase. It may be useful to submit the test bugfix commit as a separate PR, so we can see its effect on master and merge it quickly (with a tolerance bump if needed).

I checked the SciPy-specific patch instructions in eigen/arpack/ARPACK/README.scipy, but I think they are no longer needed. The *dotu and *ladiv routines don't seem to be used. slamch is already lower-case - did you apply that change, or is it part of ARPACK-NG?

We're also pulling in ~5 years worth of bug fixes, the last upgrade was in 2016. Hopefully that addresses some of the flaky test failures that we occasionally see.

Let me Cc @pv on this one to see if he has any concerns or things that need doing/checking.

@dimpase
Copy link
Contributor Author

dimpase commented Oct 3, 2021

This should be rebased over #14798 once it's merged.

@rgommers
Copy link
Member

This should be rebased over #14798 once it's merged.

That PR was merged, do you want to rebase this @dimpase?

@rgommers
Copy link
Member

Also, there's a big merge conflict since eigen moved to _eigen. I can help fix that if you can't sort that out.

@dimpase
Copy link
Contributor Author

dimpase commented Sep 16, 2023

@rgommers I no longer have nagfor (I may ask for it again, or the mighty SciPy org might ask NAG directly...),
but I am willing to proceed here one or another way, if this is desirable.

@rgommers
Copy link
Member

Hey @dimpase, I think we both lost track of this. Yes, I think this is still of interest, lots of useful changes get pulled in by upgrading to a more recent Arpack-ng (and 3.9.0 appeared 6 months ago). You don't need nagfor to finish this I think? Would you be interested in updating this PR? Once CI passes I'd be happy to merge it.

@lucascolley lucascolley added enhancement A new feature or improvement 3rd party binaries needs-work Items that are pending response from the author labels Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party binaries enhancement A new feature or improvement Fortran Items related to the internal Fortran code base needs-work Items that are pending response from the author scipy.sparse.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants