Fix out-of-bounds access for 1x1 tridiagonal matrices and add shape checks#1162
Fix out-of-bounds access for 1x1 tridiagonal matrices and add shape checks#1162srinjoy933 wants to merge 14 commits intofortran-lang:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1162 +/- ##
=======================================
Coverage 68.66% 68.66%
=======================================
Files 408 408
Lines 13619 13619
Branches 1537 1537
=======================================
Hits 9351 9351
Misses 4268 4268 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes edge cases and improves robustness in stdlib_specialmatrices tridiagonal support, addressing issue #1161 (1x1 dense conversion crash and missing dimension checks in arithmetic).
Changes:
- Guard
tridiagonal_to_dense_*against out-of-bounds access forn == 1. - Add dimension compatibility checks for tridiagonal add/sub operations.
- Extend linalg special-matrices tests to cover the 1x1 dense conversion edge case and basic arithmetic.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/specialmatrices/stdlib_specialmatrices_tridiagonal.fypp |
Prevents 1x1 out-of-bounds in dense conversion; adds dimension checks; refactors arithmetic construction paths. |
test/linalg/test_linalg_specialmatrices.fypp |
Adds tests for 1x1 dense conversion and tridiagonal arithmetic operators; updates suite naming/imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (A%n /= B%n) error stop "ValueError: tridiagonal matrices must have the same dimension to be added" | ||
|
|
||
| C = tridiagonal(A%dl + B%dl, A%dv + B%dv, A%du + B%du) |
There was a problem hiding this comment.
For consistency with the rest of the linalg/specialmatrices code, consider reporting the dimension-mismatch via linalg_state_type(..., LINALG_VALUE_ERROR, ...) + call linalg_error_handling(err0) (which is pure and already used in initialize_tridiagonal_pure_*) instead of a raw error stop with a Python-style ValueError: prefix. This will produce a standardized message including the component/location formatting used elsewhere in the library.
There was a problem hiding this comment.
C = tridiagonal(A%dl + B%dl, A%dv + B%dv, A%du + B%du) : Same as before, this will trigger unnecessary copies. Please revert this change.
Regarding the raising the error, please look at linalg_state_type and use it for consistency with the rest of stdlib.
| if (A%n /= B%n) error stop "ValueError: tridiagonal matrices must have the same dimension to be added" | ||
|
|
||
| C = tridiagonal(A%dl + B%dl, A%dv + B%dv, A%du + B%du) | ||
| end function |
There was a problem hiding this comment.
C = tridiagonal(A%dl + B%dl, ...) will typically create temporary arrays for each A%* +/- B%* expression and then copy again into C inside the constructor. If performance/memory is a concern for large n, prefer allocating/copying once (e.g., C = tridiagonal(A%dl, A%dv, A%du) after the size check) and then updating C%dl/dv/du in-place.
|
|
||
| if (A%n /= B%n) error stop "ValueError: tridiagonal matrices must have the same dimension to be subtracted" | ||
|
|
||
| C = tridiagonal(A%dl - B%dl, A%dv - B%dv, A%du - B%du) |
There was a problem hiding this comment.
Same as for addition: consider using linalg_state_type + linalg_error_handling for the dimension mismatch so error reporting is consistent across this submodule, rather than a standalone error stop string.
There was a problem hiding this comment.
Same as before. Copilot is correct again.
loiseaujc
left a comment
There was a problem hiding this comment.
Nice catch ! It never occurred to me that 1 x 1 matrices might be a particular edge case of tridiagonal matrices. It good overall, but there are a few bits and pieces that you need to revert to the original state for performance-sake, and look at linalg_state_type and linalg_error_handling to handle the errors is an stdlib-consistent way.
|
|
||
| if (A%n /= B%n) error stop "ValueError: tridiagonal matrices must have the same dimension to be added" | ||
|
|
||
| C = tridiagonal(A%dl + B%dl, A%dv + B%dv, A%du + B%du) |
There was a problem hiding this comment.
C = tridiagonal(A%dl + B%dl, A%dv + B%dv, A%du + B%du) : Same as before, this will trigger unnecessary copies. Please revert this change.
Regarding the raising the error, please look at linalg_state_type and use it for consistency with the rest of stdlib.
|
|
||
| if (A%n /= B%n) error stop "ValueError: tridiagonal matrices must have the same dimension to be subtracted" | ||
|
|
||
| C = tridiagonal(A%dl - B%dl, A%dv - B%dv, A%du - B%du) |
There was a problem hiding this comment.
Same as before. Copilot is correct again.
|
hey @loiseaujc I have made the changes that you have suggested , please have a look on this pr and suggest me if any further changes are required .Thank you! |
jvdp1
left a comment
There was a problem hiding this comment.
thank you for this PR and fix. Please find here some minor suggestions
| allocate(dl(0), dv(1), du(0)) | ||
| dv(1) = 5.0_wp |
There was a problem hiding this comment.
dl, dv and du could be defined as parameter
| allocate(dl1(n-1), dv1(n), du1(n-1)) | ||
| allocate(dl2(n-1), dv2(n), du2(n-1)) | ||
|
|
||
| dl1 = 1.0_wp ; dv1 = 2.0_wp ; du1 = 3.0_wp | ||
| dl2 = 4.0_wp ; dv2 = 5.0_wp ; du2 = 6.0_wp |
There was a problem hiding this comment.
These variables could be defined as parameter
|
hey @loiseaujc @jvdp1 I have made the changes that you have suggested please have a look at this pr,if it requires any more changes please suggest me |
jvdp1
left a comment
There was a problem hiding this comment.
Thank you @srinjoy933 for this fix. I think that it is ready to be merged.
Mahmood-Sinan
left a comment
There was a problem hiding this comment.
@srinjoy933 Here are my views.
| if (n == 1) then | ||
| B(1, 1) = A%dv(1) | ||
| else | ||
| B(1, 1) = A%dv(1) ; B(1, 2) = A%du(1) |
There was a problem hiding this comment.
| B(1, 1) = A%dv(1) ; B(1, 2) = A%du(1) | |
| B(1, 1) = A%dv(1) | |
| B(1, 2) = A%du(1) |
| B(i, i) = A%dv(i) | ||
| B(i, i+1) = A%du(i) | ||
| enddo | ||
| B(n, n-1) = A%dl(n-1) ; B(n, n) = A%dv(n) |
There was a problem hiding this comment.
| B(n, n-1) = A%dl(n-1) ; B(n, n) = A%dv(n) | |
| B(n, n-1) = A%dl(n-1) | |
| B(n, n) = A%dv(n) |
| subroutine test_tridiagonal_1x1(error) | ||
| !> Test 1x1 matrix edge case for dense conversion | ||
| type(error_type), allocatable, intent(out) :: error | ||
| #:for k1, t1, s1 in (KINDS_TYPES) |
There was a problem hiding this comment.
| #:for k1, t1, s1 in (KINDS_TYPES) | |
| #:for k1, t1, s1 in KINDS_TYPES |
Please fetch/pull once, because I've made some changes in the module. One of them was removing the brackets around KINDS_TYPES. For consistency, it would be better to remove the brackets.
| type(tridiagonal_${s1}$_type) :: A | ||
| ${t1}$, allocatable :: Amat(:,:) | ||
|
|
||
| ! Uses parameter with standard F2003 empty array syntax to satisfy maintainer |
There was a problem hiding this comment.
What do you mean by 'to satisfy maintainer' ?
There was a problem hiding this comment.
Actually I have a habit to write comments to understand what changes I have made now , when I have already done multiple commits for a particular file.I forgot to erase off those comments in this case .Nothing else. Really thank you for pointing out this mistake .
| subroutine test_tridiagonal_arithmetic(error) | ||
| !> Test arithmetic operations and optimization | ||
| type(error_type), allocatable, intent(out) :: error | ||
| #:for k1, t1, s1 in (KINDS_TYPES) |
There was a problem hiding this comment.
| #:for k1, t1, s1 in (KINDS_TYPES) | |
| #:for k1, t1, s1 in KINDS_TYPES |
| integer, parameter :: n = 3 | ||
| type(tridiagonal_${s1}$_type) :: A, B, C | ||
|
|
||
| ! Uses explicit parameter lists to satisfy maintainer and avoid 'i' compiler bug |
There was a problem hiding this comment.
Again, 'to satisfy maintainer' ?
There was a problem hiding this comment.
same cause that I have previously mentioned
| ! Check if it compiled and converted properly without segfaulting | ||
| call check(error, size(Amat, 1) == 1, .true.) |
There was a problem hiding this comment.
How can compile error be detected by this code ? If there is a compile error, that would be detected while compiling/building, not while running.
There was a problem hiding this comment.
Thank you for pointing out this. You're completely right. I've adjusted the wording to simply state: ! Check if the 1x1 matrix converted properly at runtime without segfaulting
|
hey @Mahmood-Sinan Thanks for your valuable suggestions. I have made the changes that you have suggested ,please have a look at this pr |
Mahmood-Sinan
left a comment
There was a problem hiding this comment.
@srinjoy933 Thanks for the rapid changes. A couple more changes and i think it will be good. One more suggestion is that, since the 1x1 test and arithmetic test for tridiagonal uses parameter variables, would you mind doing the same for the 1x1 test and arithmetic test for sym tridiagonal ? It would be better for consistency.
| type(tridiagonal_${s1}$_type) :: A | ||
| !! Corresponding tridiagonal matrix. | ||
|
|
||
There was a problem hiding this comment.
Please remove the trailing whitespace.
| "spmv(fail): y = alpha*A*x + beta*y, alpha: "//to_string(alpha)//", beta: "//to_string(beta)) | ||
| if (allocated(error)) return | ||
|
|
||
| ! Test y = alpha * A.T @ x beta * y for random values of alpha and beta |
There was a problem hiding this comment.
Thanks for pointing it out.
|
|
||
| testsuites = [ & | ||
| new_testsuite("specialmatrices", collect_suite) & | ||
| new_testsuite("special matrices", collect_suite) & |
There was a problem hiding this comment.
| new_testsuite("special matrices", collect_suite) & | |
| new_testsuite("special_matrices", collect_suite) & |
|
Thanks for the valuable suggestions, @Mahmood-Sinan! I'd really like to implement the same thing for symmetric tridiagonal matrices, but I think it would be best to handle that in a separate PR. If everything looks good to go here, we can merge this one and I'll tackle the symmetric tridiagonal updates next. Let me know what you think! |
|
@Mahmood-Sinan I have made the changes that you have suggested. Please have a look at this pr ,and kindly suggest if any more changes are required. Thank You! |
Mahmood-Sinan
left a comment
There was a problem hiding this comment.
Thanks you for the PR. LGTM.
solve #1161