Fix rocsparse_sptrsv_get_output singularity outputs for ELL matrices - #12010
Fix rocsparse_sptrsv_get_output singularity outputs for ELL matrices#12010kliegeois wants to merge 4 commits into
Conversation
Wire ellsv info into rocsparse_sptrsv_get_output so singular ELL matrices no longer return a false-clean none/-1 result. Co-authored-by: Cursor <cursoragent@cursor.com>
Check invariants after compute instead of re-deriving the library's symbolic/numeric precedence rule, preserve the caller's pointer mode while querying, and allocate the singularity output buffer directly since device_dense_vector cannot be instantiated on an enumeration. Co-authored-by: Cursor <cursoragent@cursor.com>
The unreleased section already announces ELL support for rocsparse_sptrsv, so the defect never shipped. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (74.68%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #12010 +/- ##
===========================================
+ Coverage 70.55% 70.56% +0.01%
===========================================
Files 2810 2810
Lines 462508 462528 +20
Branches 68099 68103 +4
===========================================
+ Hits 326292 326357 +65
+ Misses 112675 112619 -56
- Partials 23541 23552 +11
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
| @@ -175,6 +264,15 @@ void testing_sptrsv_ell(const Arguments& arg) | |||
| CHECK_ROCSPARSE_ERROR(rocsparse_status_internal_error); | |||
| } | |||
|
|
|||
| // Analysis can only expose a structural singularity, and it must be the row | |||
There was a problem hiding this comment.
Above we already query the analysis pivot so I think this is redundant. I do think that using rocsparse_sptrsv_output_singularity and rocsparse_sptrsv_output_singularity_position as you have done here is the more robust way to handle zero pivot queries with the new generic routines than using rocsparse_sptrsv_output_zero_pivot_position, but we should pick one or the other and not both.
My proposal
Given that we use rocsparse_sptrsv_output_zero_pivot_position in the other SpTRSV format testing, I suggest deleting these additions for now to keep ELL consistent with the other formats. Then in a follow-up PR, replace the usage of rocsparse_sptrsv_output_zero_pivot_position with rocsparse_sptrsv_output_singularity and rocsparse_sptrsv_output_singularity_position. This keeps this bug fix small and does not duplicate zero pivot lookup.
| @@ -237,6 +335,31 @@ void testing_sptrsv_ell(const Arguments& arg) | |||
| CHECK_ROCSPARSE_ERROR(rocsparse_status_invalid_value); | |||
| } | |||
|
|
|||
| // After compute the singularity can be structural or numeric depending on | |||
There was a problem hiding this comment.
| @@ -327,4 +450,134 @@ INSTANTIATE(int64_t, float); | |||
| INSTANTIATE(int64_t, double); | |||
| INSTANTIATE(int64_t, rocsparse_float_complex); | |||
| INSTANTIATE(int64_t, rocsparse_double_complex); | |||
| void testing_sptrsv_ell_extra(const Arguments& arg) {} | |||
|
|
|||
| static void testing_sptrsv_ell_extra_case(const host_ell_matrix<float>& hA, | |||
| @@ -347,6 +347,20 @@ try | |||
| break; | |||
| } | |||
| case rocsparse_format_ell: | |||
| { | |||
Keep testing_sptrsv_ell on the zero-pivot output used by the other formats; generic type/position coverage stays in sptrsv_ell_extra. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Problem
rocsparse_sptrsv_get_outputreturnedrocsparse_singularity_noneand position-1for ELL matrices even when analysis or compute had detected a missing or exactly-zero diagonal pivot. Therocsparse_sptrsv_output_zero_pivot_positionquery reported the pivot row correctly, so the two outputs disagreed and a caller relying on the generic singularity outputs treated a singular ELL matrix as regular.In the
rocsparse_sptrsv_output_singularity/rocsparse_sptrsv_output_singularity_positionbranch ofrocsparse_sptrsv_get_output, ELL was grouped with the formats that have no triangular-solve info (Blocked ELL, SELL, COO AoS). The symbolic and numeric pivot markers were therefore left null and the reduction produced the "no singularity" answer, while the zero-pivot branch had always used the ELL info.Fix
The ELL case now passes the pivot markers of the ELL triangular-solve info to the singularity reduction, the same way CSR, COO, and CSC do: the info object provides the symbolic marker and
get_singularity_numeric_exact()provides the numeric one. WhenBUILD_WITH_ELL_TRSVis off, the case returnsrocsparse_status_not_implemented, matching the zero-pivot branch. A null info, i.e. a query before analysis, still yieldsnoneand-1.Test
testing_sptrsv_ellcross-checks the generic singularity outputs against the zero-pivot output after analysis and after compute, in both host and device pointer mode. A newsptrsv_ell_extracase pins down the classification on three 3x3 matrices: regular, missing diagonal (symbolic at row 1 after both stages), and stored zero diagonal (none after analysis, numeric_exact at row 1 after compute).Verification
Built and run on gfx1201:
--gtest_filter=*sptrsv_ell*: 949 tests passed--gtest_filter=*extra*: 8066 tests passedsptrsv_ell_extrafails withexpected type 1 position 1, got type 0 position -1andexpected type 2 position 1, got type 0 position -1, so the new test reproduces the reported defect.JIRA ID: ROCM-30894