-
Notifications
You must be signed in to change notification settings - Fork 208
feat: Add generalized lagtm routine supporting arbitrary values for alpha and beta #1068
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
base: master
Are you sure you want to change the base?
feat: Add generalized lagtm routine supporting arbitrary values for alpha and beta #1068
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1068 +/- ##
==========================================
- Coverage 68.69% 68.60% -0.10%
==========================================
Files 392 393 +1
Lines 12693 12710 +17
Branches 1377 1377
==========================================
Hits 8720 8720
- Misses 3973 3990 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
07228b0 to
d18b50c
Compare
|
Hi @Mahmood-Sinan, this looks pretty neat! the only thing I would suggest would be to keep the files and folders naming somewhat coherent with respect to the name |
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.
Pull request overview
This PR adds a generalized glagtm routine that extends LAPACK's lagtm function to support arbitrary scalar values for alpha and beta (not just -1, 0, 1), including complex values. This enables the operation y = alpha * A * x + beta * y for tridiagonal matrices with greater flexibility.
Key Changes:
- Implemented new
glagtmsubroutine supporting arbitrary alpha/beta values for real and complex types - Updated
spmvinterface to accept complex alpha/beta parameters (previously restricted to real) - Added fallback logic to use standard LAPACK
lagtmwhen alpha/beta are special values (-1, 0, 1) for performance
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lapack_extended/stdlib_extended_lapack_base.fypp | Interface definition for new glagtm subroutine |
| src/lapack_extended/stdlib_extended_lapack.fypp | Implementation of glagtm routine handling N/T/C transpose operations |
| src/lapack_extended/CMakeLists.txt | Build configuration for new lapack_extended module |
| src/CMakeLists.txt | Integration of lapack_extended module into main build |
| src/stdlib_specialmatrices.fypp | Updated interface signatures to accept complex alpha/beta |
| src/stdlib_specialmatrices_tridiagonal.fypp | Logic to dispatch between lagtm and glagtm based on parameter values |
| test/linalg/test_linalg_specialmatrices.fypp | Added tests for random alpha/beta values across N/T/H operations |
| example/specialmatrices/example_specialmatrices_cdp_spmv.f90 | Example demonstrating complex alpha/beta usage |
| example/specialmatrices/CMakeLists.txt | Build configuration for new example |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Sure, I will do that in the next commit. |
|
I also noticed that the documentation in Since the specialmatrices routines now call glagtm when alpha or beta take arbitrary values, this warning is no longer entirely accurate. Should I update this as well? |
…ocs, updated alpha and beta checks
|
I’ve applied the changes. Please let me know if anything else is needed. |
jalvesz
left a comment
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.
LGTM @Mahmood-Sinan, thanks for this new contribution!
|
I don't really understand why the codecov jobs are failing. It would be good to understand this, maybe @loiseaujc could have an insight here? |
jvdp1
left a comment
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.
Overall LGTM. I have only a minor question
|
@jalvesz Thanks for the review. |
|
@loiseaujc @jalvesz @Mahmood-Sinan it seems that the covecod failed because the Fortran example is not covered (report here). how should we deal with that? |
The example |
|
I don't think removing the example is the solution. It would seem (and this is just a hunch) that the coverage tool is targeting the examples as sources to be analyzed in terms of coverage. The only sources to be "targeted" should be the sources within |
I agree, we should keep it. Is there a way to avoid running the examples with |
This PR adds a generalized
lagtmroutine that supports arbitrary values ofalphaandbetafor the operation:where
Ais a tridiagonal matrix. The existing LAPACKlagtmrestrictsalphaandbetato the set{ -1, 0, 1 }, which limits its applicability in several higher-level routines.The routine can be called as follows:
which is similar to the LAPACK version:
except that alpha and beta can be arbitrary(including complex values).
This new subroutine is placed inside
src/lapack_extended/stdlib_extended_lapack_base.fypp(interface) andsrc/lapack_extended/stdlib_extended_lapack.fypp(implementation).