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: linalg: Add wrapper for ?tgsyl #18571

Merged
merged 17 commits into from Jun 12, 2023
Merged

Conversation

mainak33
Copy link
Contributor

@mainak33 mainak33 commented May 28, 2023

Reference issue

Closes #18404

What does this implement/fix?

This adds a low-level wrapper for Lapack functions <s/d>tgsyl

Additional information:

  • Not final code. Help and review welcome
  • In some tests I tried to calculate lhs and rhs of the sylvester equations after solution, but they don't seem to meet the desired accuracy of machine precision*100 that is used in other tests. So, I still have to debug.
  • For now I have these questions:
  1. I'm not too sure what the input matrix forms should be. Lapack doc does say (a,b) and (d,e) should be quasi upper triangular and upper triangular respectively. Does quasi upper triangular mean block upper triangular?
  2. How do we check the matrix forms like (quasi) triangular inside the template code?
  3. I am still not sure how f2py is actually building the *.pyf.src template signature files. Does f2py itself have the mechanism to replace the templates or is there some extra build step somewhere to generate the actual *.pyf from the templates?
  4. If you are aware of other sources for tests let me know (NAG is mentioned in code. But not sure where to find it.)

@j-bowhay j-bowhay added the enhancement A new feature or improvement label May 28, 2023
@j-bowhay j-bowhay changed the title Mainak33/enh18404 ENH: linalg: Add wrapper for ?tgsyl May 28, 2023
Copy link
Member

@ilayn ilayn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mainak33 please see my comments sorry again for not looking at it earlier about lwork.

scipy/linalg/flapack_other.pyf.src Outdated Show resolved Hide resolved
scipy/linalg/flapack_other.pyf.src Outdated Show resolved Hide resolved
scipy/linalg/flapack_other.pyf.src Outdated Show resolved Hide resolved
@ilayn
Copy link
Member

ilayn commented May 29, 2023

Looks almost done to me. However the test need a full PEP8 pass then this is getting ready for merge.

@mainak33
Copy link
Contributor Author

mainak33 commented May 29, 2023

Looks almost done to me. However the test need a full PEP8 pass then this is getting ready for merge.

Fixed PEP8 styling issues with new tests in latest checkin

Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better after the PEP8 changes. Thank you

scipy/linalg/tests/test_lapack.py Outdated Show resolved Hide resolved
Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with your work, and I believe the failing tests are unrelated to your changes. Well done @mainak33! Let's wait for a final pass by @ilayn and then it should be fine

scipy/linalg/lapack.py Outdated Show resolved Hide resolved
@mainak33 mainak33 force-pushed the mainak33/ENH18404 branch 3 times, most recently from 7440d36 to 96af524 Compare May 31, 2023 13:52
@ilayn ilayn marked this pull request as ready for review May 31, 2023 14:13
@ilayn ilayn requested a review from larsoner as a code owner May 31, 2023 14:13
@ilayn
Copy link
Member

ilayn commented Jun 1, 2023

If you separate the spectrum a bit further then float32 should also get ~1e-4 level precision. So something like this should make it fly. Nevermind the scale comment above.

    atol = 1e-4 if dtype == np.float32 else 1e-10

    m, n = 10, 15

    a, _ = schur(generate_random_dtype_array([m, m], dtype=dtype))
    d = np.triu(generate_random_dtype_array([m, m], dtype=dtype))
    b, _ = schur(generate_random_dtype_array([n, n], dtype=dtype))
    e = np.triu(generate_random_dtype_array([n, n], dtype=dtype))
    c = generate_random_dtype_array([m, n], dtype=dtype)
    f = generate_random_dtype_array([m, n], dtype=dtype)

    # Adjust the spectrum of the matrices to get a more precise solution
    adjustment = np.eye(m, dtype=dtype)
    a += adjustment
    d -= 5 * adjustment
    b += adjustment
    e -= 5 * adjustment

@ilayn ilayn added this to the 1.12.0 milestone Jun 1, 2023
@mainak33
Copy link
Contributor Author

mainak33 commented Jun 1, 2023

If you separate the spectrum a bit further then float32 should also get ~1e-4 level precision. So something like this should make it fly...

I had to make a few minor changes to your suggested code due to matrix sizes but it worked on my local machine. I pushed it. Let's see if it passes all CI tests.

@mainak33
Copy link
Contributor Author

mainak33 commented Jun 1, 2023

If you separate the spectrum a bit further then float32 should also get ~1e-4 level precision. So something like this should make it fly...

I had to make a few minor changes to your suggested code due to matrix sizes but it worked on my local machine. I pushed it. Let's see if it passes all CI tests.

Tests are failing on some platforms for atol=1e-4 for single precision. Trying with atol =1e-3

@ilayn
Copy link
Member

ilayn commented Jun 1, 2023

Well these are tricky problems anyways so if it works with 1e-3 it is what it is, no need to dwell on it. If things pass, let's get it in instead of wasting your time.

@ilayn
Copy link
Member

ilayn commented Jun 1, 2023

Ah OK, the transposed problem is a different one to separate, hence the artificial push is making the 'T' option solution worse. That's why we get that silly result in float32 probably. Almost there but I'll check this tomorrow.

@mainak33
Copy link
Contributor Author

mainak33 commented Jun 2, 2023

Ah OK, the transposed problem is a different one to separate, hence the artificial push is making the 'T' option solution worse. That's why we get that silly result in float32 probably. Almost there but I'll check this tomorrow.

Thanks for looking into it. I'm afraid I don't really know the specifics of this algorithm, so won't be much help figuring out the tolerances beyond trial and error. I'll leave it upto you to fine tune the test and tolerances. Let me know if I can help otherwise or if I need to take any action once we are ready to merge the PR.

@mainak33
Copy link
Contributor Author

mainak33 commented Jun 3, 2023

The one failing test on macOS tests (meson) / Meson build (3.11) (https://github.com/scipy/scipy/actions/runs/5162854702/jobs/9300833616?pr=18571) looks like its related to #18601 ?

@ilayn
Copy link
Member

ilayn commented Jun 3, 2023

Ah dammit. I'm stupid. That's why indeed. Anyways let's wait for that one. This one is indeed ready with smaller tolerances

@mainak33
Copy link
Contributor Author

mainak33 commented Jun 3, 2023

Ah dammit. I'm stupid. That's why indeed. Anyways let's wait for that one. This one is indeed ready with smaller tolerances

No worries. I agree. This one looks ready pending the build fix for macOS. Thanks for all the help!

@mainak33
Copy link
Contributor Author

@ilayn @Kai-Striega I just rebased and pushed and it is now passing all tests with tighter tolerances that Ilayn worked on (Looks like the BLAS package updates for #18601 must have worked to fix the OSX tests for this as well). I think this is now ready for merge.

Copy link
Member

@Kai-Striega Kai-Striega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this. Waiting on @ilayn for a final review and merge.

@ilayn
Copy link
Member

ilayn commented Jun 12, 2023

Looks good, thank you very much @mainak33 for your patience and @Kai-Striega for the reviews. Let's click it

@ilayn ilayn merged commit e1f0d97 into scipy:main Jun 12, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.linalg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: Add wrapper for LAPACK functions stgsyl and dtgsyl
4 participants