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

Add tests for ?pteqr, #11224 #5

Closed

Conversation

swallan
Copy link

@swallan swallan commented Feb 25, 2020

Reference issue

Adds tests to lapack's ?pteqr wrapper: scipy#11224

What does this implement/fix?

Additional information

@mdhaber

This commit addds initial test code for pteqr as well as the real case from the NAG manual example.
This commit adds the complex exmaple from the nag manual to the pteqr test. It also reorders the parametrization.
This commit adds tests for when compz=V by creating random matrix A and then finding the tridiagonal form with ?sytrd. It modifies the NAG test to use ?hetrd with the supplied A.
this commit changes the way A is constructed for compz=V. It builds Hermitian A from Q**T * tri * Q = A by creating Q and tri.
Adds use of z for all compz cases. All random generated tests pass
@swallan
Copy link
Author

swallan commented Feb 25, 2020

Two small issues:

  • incorrect shape of z does not raise ValueError, instead prints ** On entry to SPTEQR, parameter number 6 had an illegal value. Is this expected behaviour?
  • the test from the NAG manual has a few z eigenvectors that are positive instead of negative and vice versa. The d eigenvalues are correct. I checked, and the inputs' signs are the same in the manual and in the test. Are there additional steps that I should do in the NAG test that would change the results?

Other than that, good, randomly generated tests pass.

Should I break this one up as well? The setup for this test making the inputs is what takes up the most of the test.

@mdhaber
Copy link

mdhaber commented Feb 25, 2020

the test from the NAG manual has a few z eigenvectors that are positive instead of negative and vice versa.

Generally, the only important thing about an eigenvector is a direction; the magnitude doesn't really matter. Here, we want to make sure they are all unit magnitude, but then there is still a choice of sign, so I wouldn't worry about it.

@mdhaber
Copy link

mdhaber commented Feb 25, 2020

the test from the NAG manual has a few z eigenvectors that are positive instead of negative and vice versa.

Generally, the only important thing about an eigenvector is a direction; the magnitude doesn't really matter. Here, we want to make sure they are all unit magnitude, but then there is still a choice of sign, so I wouldn't worry about it.

I'll let @Kai-Striega answer the rest.

@Kai-Striega
Copy link
Owner

Kai-Striega commented Feb 25, 2020 via email

breaks up pteqr megatest, new function to create pteqr parameters for each test, NAG manual ignore sign of z
@Kai-Striega
Copy link
Owner

@swallan the behaviour with z is correct, but a little more complicated than I had thought. For compz=N it should (and does) pass. For compz of V or I it should return with an exit code (info) of -6. This can be seen by looking at the checks near line 204:

ELSE IF( ( ldz.LT.1 ) .OR. ( icompz.GT.0 .AND. ldz.LT.max( 1,
$         n ) ) ) THEN
info = -6
END IF
IF( info.NE.0 ) THEN
CALL xerbla( 'ZPTEQR', -info )
RETURN
END IF

However, the "correct" behaviour can lead to segfaults when reading the z matrix. So I'm inclined to write a check in spite of the default behaviour. @mdhaber do you have any thoughts on this?

Also while looking at your tests it looks like there is another error. This just looks like a very small tolerance error:

=================================== FAILURES ===================================
_______________________ test_pteqr[V-complex128-float64] _______________________
[gw3] linux -- Python 3.7.5 /mnt/c/Users/Kai/develop/venv-wsl/scipy37/bin/python
scipy/linalg/tests/test_lapack.py:1839: in test_pteqr
    rtol=rtol, atol=atol)
E   AssertionError: 
E   Not equal to tolerance rtol=5.55112e-14, atol=2.22045e-14
E   
E   Mismatched elements: 2 / 100 (2%)
E   Max absolute difference: 2.49922314e-14
E   Max relative difference: 2.39808173e-14
E    x: array([[ 1.000000e+00+0.000000e+00j, -4.996004e-16-1.631291e-15j,
E            4.215378e-16-7.799317e-15j, -9.575674e-16+8.264223e-15j,
E           -1.006140e-15-7.299716e-15j,  1.249001e-15-3.771289e-15j,...
E    y: array([[1., 0., 0., 0., 0., 0., 0., 0., 0., 0.],
E          [0., 1., 0., 0., 0., 0., 0., 0., 0., 0.],
E          [0., 0., 1., 0., 0., 0., 0., 0., 0., 0.],...
________________________ test_pteqr[V-float64-float64] _________________________
[gw1] linux -- Python 3.7.5 /mnt/c/Users/Kai/develop/venv-wsl/scipy37/bin/python
scipy/linalg/tests/test_lapack.py:1839: in test_pteqr
    rtol=rtol, atol=atol)
E   AssertionError: 
E   Not equal to tolerance rtol=5.55112e-14, atol=2.22045e-14
E   
E   Mismatched elements: 20 / 100 (20%)
E   Max absolute difference: 5.59005217e-14
E   Max relative difference: 2.80886425e-14
E    x: array([[ 1.000000e+00, -2.258285e-14,  9.830301e-15, -1.450336e-14,
E            7.119861e-15, -1.386724e-14,  8.246308e-16, -5.143612e-15,
E           -2.495473e-15,  3.792739e-14],...
E    y: array([[1., 0., 0., 0., 0., 0., 0., 0., 0., 0.],
E          [0., 1., 0., 0., 0., 0., 0., 0., 0., 0.],
E          [0., 0., 1., 0., 0., 0., 0., 0., 0., 0.],...

@swallan
Copy link
Author

swallan commented Feb 27, 2020

@Kai-Striega I will loosen the tolerance a little bit! I don't get those issues on my my system, but I'll bump it from e-14 to e-13.

@mdhaber
Copy link

mdhaber commented Feb 27, 2020

So I'm inclined to write a check in spite of the default behaviour.

Yes, I think it's best to catch it early! We don't want the possibility of a segfault if we can avoid it.

@mdhaber
Copy link

mdhaber commented Mar 8, 2020

@swallan it doesn't look like you changed the tolerances. Otherwise, how's this looking @Kai-Striega? If you're able to make the tolerance change yourself, are they passing tests?

@mdhaber
Copy link

mdhaber commented Mar 12, 2020

See scipy#11665

Kai-Striega pushed a commit that referenced this pull request Jun 22, 2020
Kai-Striega pushed a commit that referenced this pull request Aug 26, 2021
Kai-Striega pushed a commit that referenced this pull request Feb 18, 2024
* MAINT:special:amos:Remove AMOS Fortran library

[skip ci]

* ENH:special:amos:Rewrite public airy, biry, bes(h,i,j,k,y)

[skip ci]

ENH:special:amos:Rewrite airy in C

[skip ci]

ENH:special:amos:Rewrite biry in C

[skip ci]

ENH:special:amos: Rewrite acon, bunk, unk1, unk2 in C

[skip ci]

* BLD:special:amos:Restructure and add define macros

[skip ci]

BLD:special:amos:Rename amos functions with prefix

[skip ci]

BUG:MAINT:special:amos:Initialize complaining variables

[skip ci]

FIX:special:amos: Fix two bugs for asyi and besh

[skip ci]

* BLD:special:amos:Modify amos wrappers for the C versions

[sikp ci]

* BLD:special:amos:Modify meson file for C sources

* MAINT:special:amos:Fix typo in amos_wrappers

[skip ci]

* ENH:special:amos:Major iteration on _amos.c

ENH:special:amos:Major iteration on _amos.c

[skip ci]

FIX:special:amos: Avoid stack smashing problem on Linux

Also removed one extra GO TO

BLD:special:amos:Declare helper functions as static

* TST:special:Use proper integer type in  TestBessel

* MAINT:special:amos: Use CMPLX and npy_c macros in the wrappers

* Remove a few goto (#5)

* MAINT: special: remove goto in acai

* MAINT: special: remove some gotos from bknu

* MAINT: simplify if statements

* MAINT: special: remove a few gotos unk1

* MAINT: remove duplicate code in acai

* STY:special:amos:Basic fixups for do loops

---------

Co-authored-by: Ilhan Polat <ilhanpolat@gmail.com>

* DOC:special:amos:Add copyright, license and docstrings

[skip ci]

* STY:special:amos: Cleanup data and comments, silence warnings

MAINT:special:amos: Add const specifier for array data

STY:special:amos: Cleanup and organize data and comments

MAINT:special:amos: Silence compiler for fabs(integer) use

* MAINT:special:amos: Address review comments

---------

Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants