Skip to content

Improve solveCubic accuracy#27347

Merged
asmorkalov merged 3 commits intoopencv:4.xfrom
MaximSmolskiy:improve-solveCubic-accuracy
May 24, 2025
Merged

Improve solveCubic accuracy#27347
asmorkalov merged 3 commits intoopencv:4.xfrom
MaximSmolskiy:improve-solveCubic-accuracy

Conversation

@MaximSmolskiy
Copy link
Copy Markdown
Contributor

Pull Request Readiness Checklist

Fix #27323

2e-13 * x^3 + x^2 - 2 * x + 1 = 0 -> x^3 + 5e12 * x^2 - 1e13 * x + 5e12 = 0

The problem that coefficients have quite big magnitudes and current calculations are subject to round-off error

Q = (a1 * a1 - 3 * a2) * (1./9)
R = (2 * a1 * a1 * a1 - 9 * a1 * a2 + 27 * a3) * (1./54)
Qcubed = Q * Q * Q = a1^6/729 - (a1^4 a2)/81 + (a1^2 a2^2)/27 - a2^3/27
R * R = R^2 = a1^6/729 - (a1^4 a2)/81 + (a1^2 a2^2)/36 + (a1^3 a3)/27 - (a1 a2 a3)/6 + a3^2/4
d = Qcubed - R * R

Let a1, a2, a3 have quite big same magnitudes, then we see that Qcubed and R * R have same terms a1^6/729 and -(a1^4 a2)/81 (which will be reduced in d), but they level out the other terms (these terms have 6th and 5th degree and other terms - less or equal than 4th degree).
So, if these terms will participate in the calculation, this will lead to a huge round-off error.
But if we expand the expression, then round-off error should be less

d = Qcubed - R * R = 1/108 (a1^2 a2^2 - 4 a2^3 - 4 a1^3 a3 + 18 a1 a2 a3 - 27 a3^2)

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@vpisarev
Copy link
Copy Markdown
Contributor

@MaximSmolskiy, thank you for the patch! I think, generally this is the right direction to modify the function, and in this particular example it helps, but can it be improved even further in the same patch?

Generally, when you have (K*a*b - K*a*c)/(N*K), it makes sense to reduce it to (b-c)*a/N, because the largest source of rounding errors is when you add or subtract two big numbers or when you divide one big number by another big one, and so the methodology to reduce rounding errors is to reduce (to a reasonable level) magnitude of values that you add/subtract.

In particular, the last part of d formula is:

double d = ... + (18 * a1 * a2 * a3 - 27 * a3 * a3) * (1./108);

which could be transformed to

double d = ... + (2 * a1 * a2 - 3 * a3) * (a3/12);

here you greatly reduce magnitude of the two subtracted items and thus probably achieve higher accuracy. Probably the first part of d formula can be simplified as well.

Numeric methods are always very tricky, and probably here we need to spend a little more time in order to improve accuracy in most cases, not only in some cases.

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

@vpisarev I grouped terms with same total degree

@asmorkalov
Copy link
Copy Markdown
Contributor

Looks good to me in general. It'll be great to add a comment with the original formulae and the reason why the parts are grouped.

@MaximSmolskiy
Copy link
Copy Markdown
Contributor Author

Added brief comment

@asmorkalov asmorkalov added this to the 4.12.0 milestone May 24, 2025
@asmorkalov asmorkalov merged commit 023d14e into opencv:4.x May 24, 2025
25 of 28 checks passed
@MaximSmolskiy MaximSmolskiy deleted the improve-solveCubic-accuracy branch May 24, 2025 08:20
@asmorkalov asmorkalov mentioned this pull request May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

solveCubic if a0 is too small(like 10^-13), the solution is abnormal.

3 participants