Add support for negating a complex number #193
Conversation
kgryte
changed the title
kgryte
added
Feature
There was a problem hiding this comment.
@ShraddheyaS Thanks for working on this! Main questions center around positive and negative zeros. Once these are resolved, this PR should be ready for acceptance.
|
|
||
| v = cnegate( 0.0, 0.0 ); | ||
| // returns [ TODO, TODO ] | ||
| // returns [ 0.0, 0.0 ] |
There was a problem hiding this comment.
Is this correct? Why not negative zeros?
| * var out = new Array( 2 ); | ||
| * var v = cnegate( out, 0.0, 0.0 ); | ||
| * // returns [ TODO, TODO ] | ||
| * // returns [ 0.0, 0.0 ] |
| * | ||
| * v = cnegate( 0.0, 0.0 ); | ||
| * // returns [ TODO, TODO ] | ||
| * // returns [ 0.0, 0.0 ] |
| * @example | ||
| * var v = cnegate( 0.0, 0.0 ); | ||
| * // returns [ TODO, TODO ] | ||
| * // returns [ 0.0, 0.0 ] |
| t.deepEqual( actual, expected, 'returns expected value' ); | ||
|
|
||
| actual = cnegate( 0.0, 0.0 ); | ||
| expected = [ 0.0, 0.0 ]; |
There was a problem hiding this comment.
This test is a bit more nuanced, as one needs to test for negative/positive zeros. Ideally, we would have tests for both.
There was a problem hiding this comment.
I believe this test is actually already taken care of in the tests starting at L118 and L125. Feel free to modify this test with, say, two negative numbers.
There was a problem hiding this comment.
Yes, thank you for pointing it out. I saw the isPositiveZero and isNegativeZero tests now. I have changed these tests in the latest commit.
|
|
||
| actual = cnegate( 0.0, 0.0 ); | ||
| expected = [ -0.0, -0.0 ]; | ||
| t.deepEqual( actual, expected, 'returns expected value' ); |
There was a problem hiding this comment.
Unfortunately, deepEqual does not check for sign equality when comparing zeros. Hence, the following test passes
actual = cnegate( 0.0, 0.0 );
expected = [ 0.0, 0.0 ];
t.deepEqual( actual, expected, 'returns expected value' );even if it shouldn't. Unfortunately, one cannot use deepEqual, but, instead, one must manually check individual array elements. In this case, that entails checking for value and sign equality. This has already been done, in later tests. So having tests here which test function signed zero behavior is not necessary. Perhaps, better might be to just test other values, perhaps with a different sign combination. For example,
actual = cnegate( -1.0, -1.0 );
expected = [ 1.0, 1.0 ];
t.deepEqual( actual, expected, 'returns expected value' );There was a problem hiding this comment.
Yes, I realized the redundancy and changed the tests.
There was a problem hiding this comment.
@ShraddheyaS Looks good! Will merge once the canary build passes.
* Add pkg scaffold * Fix pkg path * Add support for negating a complex number (#193) * Completed all the TODO's * Fixed the minus zero results * Edited the redundant tests which are being checked later
2 participants
Resolves #101.
Checklist
develop.developbranch.Description
This pull request:
Related Issues
This pull request:
Questions
No.
Other
Yes.
feature/complex-negatebranch.@stdlib-js/reviewers