SolveNormalizedCubic fix to return proper real root #224
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com>
bareya
force-pushed
the
solve_normalized_cubic
branch
from
9e487eb to
f912b0c
Compare
meshula
reviewed
Oct 28, 2021
src/Imath/ImathRoots.h
Outdated
| if (D > 0) | ||
| { | ||
| auto real_root = [] (T a, T x) -> T { | ||
| T sign = a < T (0) ? T (-1) : T (1); |
There was a problem hiding this comment.
Suggested change
| T sign = a < T (0) ? T (-1) : T (1); | |
| T sign = std::copysign(T(1), a); |
There was a problem hiding this comment.
Thanks for the suggestion @meshula.
Also, I have a question, I didn't notice but in the declaration of the function here:
https://github.com/AcademySoftwareFoundation/Imath/blob/master/src/Imath/ImathRoots.h#L67
IMATH_HOSTDEVICE is used. Does it mean we can't use std:: features? I guess code has to compile as cuda code too?
There was a problem hiding this comment.
Apologies for the delay, and thank you for the fix! I'm not entirely sure about the complete details of compatibility between cuda and std, but I'm pretty sure simple inline std:: functions are acceptable.
use copy sign Co-authored-by: Nick Porcino <meshula@hotmail.com> Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com>
bareya
force-pushed
the
solve_normalized_cubic
branch
from
f3a6075 to
e2fe4b7
Compare
cary-ilm
pushed a commit
to cary-ilm/Imath
that referenced
this pull request
Jan 17, 2022
…oundation#224) * SolveNormalizedCubic fix to return proper real root Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com> * Update src/Imath/ImathRoots.h use copy sign Co-authored-by: Nick Porcino <meshula@hotmail.com> Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com> Co-authored-by: Nick Porcino <meshula@hotmail.com>
cary-ilm
pushed a commit
that referenced
this pull request
Jan 20, 2022
* SolveNormalizedCubic fix to return proper real root Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com> * Update src/Imath/ImathRoots.h use copy sign Co-authored-by: Nick Porcino <meshula@hotmail.com> Signed-off-by: Piotr Barejko <piotrbarejko@protonmail.com> Co-authored-by: Nick Porcino <meshula@hotmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some cases
solveNormalizedCubiccan return a real value from principled root that is a complex root. This happens in #86 for example.In case of discriminant
D > 0there must be one real and two complex roots. Given formula:-q/2 + sqrt(D)might be a negative number. Instead of searching thoughy0,y1,y2to find the real root, we can solve it as follows: