[OpenVINO backend] solve_triangular implementation for OpenVINO backend#22588
[OpenVINO backend] solve_triangular implementation for OpenVINO backend#22588satheeshbhukya wants to merge 1 commit intokeras-team:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the solve_triangular operation for the OpenVINO backend and enables the corresponding correctness tests. The implementation handles input conversion, rank alignment, and uses a masking strategy to ensure the matrix is triangular before applying matrix inversion and multiplication. Review feedback highlights the need for documentation regarding the precision loss caused by downcasting f64 to f32 and the numerical stability and performance trade-offs associated with using matrix inversion instead of standard substitution methods.
| if orig_type == Type.f64: | ||
| a_ov = ov_opset.convert(a_ov, Type.f32).output(0) | ||
| b_ov = ov_opset.convert(b_ov, Type.f32).output(0) |
There was a problem hiding this comment.
The downcasting from f64 to f32 here introduces a loss of precision. While the PR description explains this is a workaround for ov_opset.inverse lacking f64 support, this critical information should be captured in a code comment for future maintainers.
Please add a comment explaining why this conversion is necessary. For example:
# `ov_opset.inverse` does not support f64, so we downcast to f32
# and cast back the result later.| a_inv = ov_opset.inverse(a_ov, adjoint=False).output(0) | ||
| result = ov_opset.matmul(a_inv, b_ov, False, False).output(0) |
There was a problem hiding this comment.
Using matrix inversion followed by matrix multiplication to solve a triangular system is generally less numerically stable and less performant (O(n³) vs. O(n²)) than using forward or backward substitution.
If OpenVINO does not provide a more direct way to solve triangular systems (e.g., via a substitution op), this implementation is a reasonable workaround. However, this trade-off is significant, especially since the inputs are already downcast from f64 to f32, which can further impact numerical accuracy.
Consider adding a comment here to document this implementation choice and its potential drawbacks for future reference.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22588 +/- ##
==========================================
- Coverage 83.30% 83.29% -0.01%
==========================================
Files 596 596
Lines 67951 67982 +31
Branches 10577 10582 +5
==========================================
+ Hits 56604 56628 +24
- Misses 8600 8603 +3
- Partials 2747 2751 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
This PR adds support for solve_triangular (solving the linear system AX = B where A is a triangular matrix) in the OpenVINO backend.
Implementation
The operation is implemented using matrix inversion and multiplication, built entirely with OpenVINO ops.
The main steps are:
closes: openvinotoolkit/openvino/issues/#35002
cc @hertschuh for review