Skip to content

Update polynom_solver.cpp#19583

Merged
alalek merged 2 commits intoopencv:3.4from
mradul2:patch-1
Mar 5, 2021
Merged

Update polynom_solver.cpp#19583
alalek merged 2 commits intoopencv:3.4from
mradul2:patch-1

Conversation

@mradul2
Copy link
Copy Markdown
Contributor

@mradul2 mradul2 commented Feb 20, 2021

This pull request is in the response to Issue #19526. I have fixed the problem with the cube root calculation of 2*R. The Issue was in the usage of pow function with negative values of R, but if it is calculated for only positive values of R then changing x0 according to the parity of R, the Issue is resolved. Kindly consider it, Thanks!

Pull Request Readiness Checklist

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 other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to 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

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for contribution!

As a "bugfix" this patch should go into 3.4 branch first.
We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

Please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

Comment on lines +67 to +71
else {
x0 = pow(2 * R, 1 / 3.0) - b_a_3;
return 1;
double cube_root = pow(abs(2 * R), (1.0 / 3.0));
if(R<0)
{
x0 = -cube_root - b_a_3;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep existed indentation coding style here (2 spaces in this file)

@catree
Copy link
Copy Markdown
Contributor

catree commented Feb 22, 2021

There is also std::cbrt() (since C++11):

Notes
std::cbrt(arg) is not equivalent to std::pow(arg, 1.0/3) because the rational number 1/3 is typically not equal to 1.0/3 and std::pow cannot raise a negative base to a fractional exponent. Moreover, std::cbrt(arg) usually gives more accurate results than std::pow(arg, 1.0/3) (see example).

@asmorkalov
Copy link
Copy Markdown
Contributor

@theroyalpekka Friendly reminder.

mradul2 and others added 2 commits March 3, 2021 12:31
This pull request is in the response to Issue  #19526. I have fixed the problem with the cube root calculation of 2*R. The Issue was in the usage of pow function with negative values of R, but if it is calculated for only positive values of R then changing x0 according to the parity of R, the Issue is resolved. Kindly consider it, Thanks!
@alalek alalek merged commit 640f188 into opencv:3.4 Mar 5, 2021
@mradul2 mradul2 deleted the patch-1 branch March 5, 2021 13:58
@alalek alalek mentioned this pull request Mar 6, 2021
@alalek alalek mentioned this pull request Apr 9, 2021
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.

solve_deg3 gives incorrect result (polynom_solver.cpp)

4 participants