opencv icon indicating copy to clipboard operation
opencv copied to clipboard

core: fix solveCubic numerical instability via coefficient normalization (fixes #27748)

Open satyam102006 opened this issue 4 months ago • 7 comments

Summary This PR fixes numerical instability in cv::solveCubic when the leading coefficient a is non-zero but extremely small relative to other coefficients (Issue #27748).

It introduces a normalization step that scales all coefficients by their maximum magnitude before solving. This ensures robust detection of when the equation should degenerate to a quadratic solver, without breaking valid cubic equations that happen to have small coefficients (e.g., scaled by 1e-9).

The Problem (Issue #27748) The previous implementation checked if (a == 0) to decide whether to use the cubic or quadratic formula.

  • When a is extremely small (e.g., 1e-17) but not exactly zero, and other coefficients are normal (e.g., 5.0), the standard cubic formula suffers from catastrophic cancellation and overflow, producing incorrect roots (e.g., 1e14).

The Fix

  1. Normalization: The solver now finds max_coeff = max(|a|, |b|, |c|, |d|) and scales all coefficients by 1.0 / max_coeff.
  2. Relative Threshold: It then checks if (abs(a) < epsilon) on the normalized coefficients.

Why this is better than previous attempts In a previous attempt (PR #28057), a simple absolute check abs(a) < epsilon was proposed. That approach was rejected because it failed for scaled equations .

Fixes #27748

satyam102006 avatar Dec 02 '25 14:12 satyam102006

The PR introduce regression:

[ RUN      ] Core_SolveCubicConstant.accuracy
  /home/ci/opencv/modules/core/test/test_math.cpp:2464: Failure
  Expected equality of these values:
    num_roots
      Which is: 0
    -1
  [  FAILED  ] Core_SolveCubicConstant.accuracy (0 ms)
  [ RUN      ] Core_SolveCubicQuadratic.accuracy
  /home/ci/opencv/modules/core/test/test_math.cpp:2503: Failure
  Expected equality of these values:
    roots[0]
      Which is: 2
    2.
      Which is: 2
  /home/ci/opencv/modules/core/test/test_math.cpp:2504: Failure
  Expected equality of these values:
    roots[1]
      Which is: 1
    1.
      Which is: 1
  [  FAILED  ] Core_SolveCubicQuadratic.accuracy (0 ms)
  [ RUN      ] Core_SolveCubic.regression_27323
  /home/ci/opencv/modules/core/test/test_math.cpp:2608: Failure
  Expected equality of these values:
    roots[0]
      Which is: 1
    -5e12 - 2.
      Which is: -5e+12
  /home/ci/opencv/modules/core/test/test_math.cpp:2617: Failure
  Expected equality of these values:
    roots[0]
      Which is: 1
    -5e12 - 2.
      Which is: -5e+12
  [  FAILED  ] Core_SolveCubic.regression_27323 (0 ms)

Please also add test case for the issue you are solving.

asmorkalov avatar Dec 03 '25 08:12 asmorkalov

The PR introduce regression:

[ RUN      ] Core_SolveCubicConstant.accuracy
  /home/ci/opencv/modules/core/test/test_math.cpp:2464: Failure
  Expected equality of these values:
    num_roots
      Which is: 0
    -1
  [  FAILED  ] Core_SolveCubicConstant.accuracy (0 ms)
  [ RUN      ] Core_SolveCubicQuadratic.accuracy
  /home/ci/opencv/modules/core/test/test_math.cpp:2503: Failure
  Expected equality of these values:
    roots[0]
      Which is: 2
    2.
      Which is: 2
  /home/ci/opencv/modules/core/test/test_math.cpp:2504: Failure
  Expected equality of these values:
    roots[1]
      Which is: 1
    1.
      Which is: 1
  [  FAILED  ] Core_SolveCubicQuadratic.accuracy (0 ms)
  [ RUN      ] Core_SolveCubic.regression_27323
  /home/ci/opencv/modules/core/test/test_math.cpp:2608: Failure
  Expected equality of these values:
    roots[0]
      Which is: 1
    -5e12 - 2.
      Which is: -5e+12
  /home/ci/opencv/modules/core/test/test_math.cpp:2617: Failure
  Expected equality of these values:
    roots[0]
      Which is: 1
    -5e12 - 2.
      Which is: -5e+12
  [  FAILED  ] Core_SolveCubic.regression_27323 (0 ms)

Please also add test case for the issue you are solving.

Thank you for the feedback.

  1. Fixed Regressions: Core_SolveCubicQuadratic (Precision):I modified the logic to use the original (raw) coefficients when falling back to the quadratic solver. This prevents the floating-point rounding errors that were causing the exact-match tests to fail. Core_SolveCubicConstant (Infinite Roots): Added an explicit check for all-zero coefficients to correctly return -1 (infinite roots) as expected by the tests. Core_SolveCubic.regression_27323 (Small Valid Cubic): I updated the threshold logic to use std::numeric_limits<double>::epsilon() on normalized coefficients. This ensures that valid cubic equations with small but significant terms are not incorrectly degraded to quadratic.

  2. Test Case Added: I have added TEST(Core_SolveCubic, regression_27748) to test_math.cpp, which specifically targets the numerical instability reported in the issue.

  3. CI Fix: Added missing <algorithm> and <cmath> headers to fix the build failures on Linux/Windows CI agents.

satyam102006 avatar Dec 03 '25 14:12 satyam102006

CI Warnings:

**modules/core/src/mathfuncs.cpp:49: trailing whitespace.
+#include <algorithm> 
modules/core/src/mathfuncs.cpp:50: trailing whitespace.
+#include <cmath>     
modules/core/src/mathfuncs.cpp:1606: trailing whitespace.
+        // CRITICAL: Use RAW coefficients here to avoid rounding errors in trivial cases 
modules/core/src/mathfuncs.cpp:1650: trailing whitespace.
+        double a0 = a0_norm; 
modules/core/src/mathfuncs.cpp:1663: trailing whitespace.
+        
modules/core/test/test_math.cpp:2623: trailing whitespace.
+    
modules/core/test/test_math.cpp:2624: trailing whitespace.
+    // a is extremely small relative to others (approx 1.8e-19 ratio), 
modules/core/test/test_math.cpp:2633: trailing whitespace.
+    
modules/core/test/test_math.cpp:2638: trailing whitespace.
+    
modules/core/test/test_math.cpp:2642: trailing whitespace.
+    
modules/core/test/test_math.cpp:2646: trailing whitespace.
+        EXPECT_NEAR(r, 0.5, 1.0); 

asmorkalov avatar Dec 04 '25 08:12 asmorkalov

CI Warnings:

**modules/core/src/mathfuncs.cpp:49: trailing whitespace.
+#include <algorithm> 
modules/core/src/mathfuncs.cpp:50: trailing whitespace.
+#include <cmath>     
modules/core/src/mathfuncs.cpp:1606: trailing whitespace.
+        // CRITICAL: Use RAW coefficients here to avoid rounding errors in trivial cases 
modules/core/src/mathfuncs.cpp:1650: trailing whitespace.
+        double a0 = a0_norm; 
modules/core/src/mathfuncs.cpp:1663: trailing whitespace.
+        
modules/core/test/test_math.cpp:2623: trailing whitespace.
+    
modules/core/test/test_math.cpp:2624: trailing whitespace.
+    // a is extremely small relative to others (approx 1.8e-19 ratio), 
modules/core/test/test_math.cpp:2633: trailing whitespace.
+    
modules/core/test/test_math.cpp:2638: trailing whitespace.
+    
modules/core/test/test_math.cpp:2642: trailing whitespace.
+    
modules/core/test/test_math.cpp:2646: trailing whitespace.
+        EXPECT_NEAR(r, 0.5, 1.0); 

Apologies for the build failures. I will pushed a fix that:

  1. Removes trailing whitespace to pass the style check.
  2. Adds missing headers (<algorithm>, <cmath>) for cross-platform compilation.

satyam102006 avatar Dec 04 '25 13:12 satyam102006

Sorry I used ai for removing whitespace which occur in docs previous once again sorry for forward I will make sure of this.

For inconvenience you can close my PR

On Thu, 4 Dec 2025, 23:38 s-trinh, @.***> wrote:

@.**** commented on this pull request.

In modules/core/src/mathfuncs.cpp https://github.com/opencv/opencv/pull/28117#discussion_r2590080223:

  •    /*
    
  •      Here we expand expression `Qcubed - R * R` for `d` variable
    
  •      to reduce common terms `a1^6 / 729` and `-a1^4 * a2 / 81`
    
  •      and thus decrease rounding error (in case of quite big coefficients).
    
  •      And then we additionally group terms to further reduce rounding error.
    
  •    */
    

Please, do not remove old comments.

BTW, too much AI code / comments. No idea if the approach is valid or not, or if the comments match with the code.

— Reply to this email directly, view it on GitHub https://github.com/opencv/opencv/pull/28117#pullrequestreview-3541389583, or unsubscribe https://github.com/notifications/unsubscribe-auth/BMZMNSHLGSG422EJ6SKHPV34AB2B5AVCNFSM6AAAAACNZXSE46VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBRGM4DSNJYGM . You are receiving this because you authored the thread.Message ID: @.***>

satyam102006 avatar Dec 04 '25 18:12 satyam102006

Sure I will remember all these things

On Fri, 5 Dec 2025, 10:54 Alexander Smorkalov, @.***> wrote:

@.**** commented on this pull request.

In modules/core/test/test_math.cpp https://github.com/opencv/opencv/pull/28117#discussion_r2591438794:

  • Mat coeffs = (Mat_(1, 4) << a, b, c, d);
  • Mat roots;
  • // We expect the solver to detect the negligible 'a' and solve as quadratic:
  • // 84.4504x^2 - 96.795x + 13.6826 = 0
  • // Discriminant ~ 4745.8
  • // Roots approx: 0.156, 0.990
  • int n = solveCubic(coeffs, roots);
  • EXPECT_GE(n, 2); // Should find at least 2 real roots (as quadratic)
  • for(int i = 0; i < n; i++)
  • {
  •    double r = roots.at<double>(i);
    
  •    EXPECT_NEAR(r, 0.5, 1.0);
    

It means that roots are validated in +-1 range. It's too much. See documentation: https://google.github.io/googletest/reference/assertions.html#EXPECT_NEAR

In modules/core/src/mathfuncs.cpp https://github.com/opencv/opencv/pull/28117#discussion_r2591442231:

  •        double theta = acos(R / sqrt(Qcubed));
    
  •        double sqrtQ = sqrt(Q);
    

Please do not add std:: to the code you do not touch. It produces a lot of noise.

— Reply to this email directly, view it on GitHub https://github.com/opencv/opencv/pull/28117#pullrequestreview-3543151102, or unsubscribe https://github.com/notifications/unsubscribe-auth/BMZMNSFHA2JXR2MTRGG2NA34AEJKFAVCNFSM6AAAAACNZXSE46VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBTGE2TCMJQGI . You are receiving this because you authored the thread.Message ID: @.***>

satyam102006 avatar Dec 05 '25 05:12 satyam102006

Ok, I will do that.

On Fri, 5 Dec 2025, 13:06 Vadim Pisarevsky, @.***> wrote:

@.**** requested changes on this pull request.

please, reduce the patch size as much as possible, i.e. remove unnecessary changes, makes the comments less verbose, keep the old code and comments where it's possible.

— Reply to this email directly, view it on GitHub https://github.com/opencv/opencv/pull/28117#pullrequestreview-3543454253, or unsubscribe https://github.com/notifications/unsubscribe-auth/BMZMNSAZGFR5UXWSIF6BB6L4AEYYVAVCNFSM6AAAAACNZXSE46VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBTGQ2TIMRVGM . You are receiving this because you authored the thread.Message ID: @.***>

satyam102006 avatar Dec 05 '25 07:12 satyam102006