Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add math/base/special/cinvf #3103

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from

Conversation

aayush0325
Copy link
Member

@aayush0325 aayush0325 commented Nov 12, 2024

Resolves a part of #649

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Need help with overflow case.

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Nov 12, 2024
@aayush0325
Copy link
Member Author

aayush0325 commented Nov 12, 2024

tape( 'the function may overflow', function test( t ) {
	var v;

	v = cinvf( new Complex64( 5.0e-324, 5.0e-324 ) );
	t.strictEqual( real( v ), PINF, 'real component is +infinity' );
	t.strictEqual( imag( v ), NINF, 'imaginary component is -infinity' );

	v = cinvf( new Complex64( -5.0e-324, 5.0e-324 ) );
	t.strictEqual( real( v ), NINF, 'real component is -infinity' );
	t.strictEqual( imag( v ), NINF, 'imaginary component is -infinity' );

	v = cinvf( new Complex64( -5.0e-324, -5.0e-324 ) );
	t.strictEqual( real( v ), NINF, 'real component is -infinity' );
	t.strictEqual( imag( v ), PINF, 'imaginary component is +infinity' );

	v = cinvf( new Complex64( 5.0e-324, -5.0e-324 ) );
	t.strictEqual( real( v ), PINF, 'real component is +infinity' );
	t.strictEqual( imag( v ), PINF, 'imaginary component is +infinity' );

	v = cinvf( new Complex64( 0.0, 5.0e-324 ) );
	t.strictEqual( real( v ), 0.0, 'real component is 0' );
	t.strictEqual( imag( v ), NINF, 'imaginary component is -infinity' );

	v = cinvf( new Complex64( 0.0, -5.0e-324 ) );
	t.strictEqual( real( v ), 0.0, 'real component is 0' );
	t.strictEqual( imag( v ), PINF, 'imaginary component is +infinity' );

	v = cinvf( new Complex64( 5.0e-324, 0.0 ) );
	t.strictEqual( real( v ), PINF, 'real component is +infinity' );
	t.strictEqual( imag( v ), 0.0, 'imaginary component is 0' );

	v = cinvf( new Complex64( -5.0e-324, 0.0 ) );
	t.strictEqual( real( v ), NINF, 'real component is -infinity' );
	t.strictEqual( imag( v ), 0.0, 'imaginary component is 0' );

	t.end();
});

Good Afternoon, @gunjjoshi can you please help me out in adding the overflow testcase in test.js and test.native.js as it is not there in this PR right now, i tried various values for this but the complex number real( v ) imag( v ) turns out to be NaN NaN and not Infinity or -Infinity as expected. how did you arrive at the number 5.0e-324?

@gunjjoshi
Copy link
Member

gunjjoshi commented Nov 12, 2024

@aayush0325 Tried the following test on your branch, and it works:

tape( 'the function may produce very large values for very small inputs', function test( t ) {
    var v;

    v = cinvf( new Complex64( FLOAT32_SMALLEST_NORMAL, FLOAT32_SMALLEST_NORMAL ) );
    t.ok( real( v ) > 1e37, 'real component is very large' );
    t.ok( imag( v ) < -1e37, 'imaginary component is very large and negative' );

    v = cinvf( new Complex64( -FLOAT32_SMALLEST_NORMAL, FLOAT32_SMALLEST_NORMAL ) );
    t.ok( real( v ) < -1e37, 'real component is very large and negative' );
    t.ok( imag( v ) < -1e37, 'imaginary component is very large and negative' );

    v = cinvf( new Complex64( -FLOAT32_SMALLEST_NORMAL, -FLOAT32_SMALLEST_NORMAL ) );
    t.ok( real( v ) < -1e37, 'real component is very large and negative' );
    t.ok( imag( v ) > 1e37, 'imaginary component is very large and positive' );

    v = cinvf( new Complex64( FLOAT32_SMALLEST_NORMAL, -FLOAT32_SMALLEST_NORMAL ) );
    t.ok( real( v ) > 1e37, 'real component is very large and positive' );
    t.ok( imag( v ) > 1e37, 'imaginary component is very large and positive' );

    v = cinvf( new Complex64( 0.0, FLOAT32_SMALLEST_NORMAL ) );
    t.strictEqual( real( v ), 0.0, 'real component is 0' );
    t.ok( imag( v ) < -1e37, 'imaginary component is very large and negative' );

    v = cinvf( new Complex64( 0.0, -FLOAT32_SMALLEST_NORMAL ) );
    t.strictEqual( real( v ), 0.0, 'real component is 0' );
    t.ok( imag( v ) > 1e37, 'imaginary component is very large and positive' );

    v = cinvf( new Complex64( FLOAT32_SMALLEST_NORMAL, 0.0 ) );
    t.ok( real( v ) > 1e37, 'real component is very large and positive' );
    t.strictEqual( imag( v ), 0.0, 'imaginary component is 0' );

    v = cinvf( new Complex64( -FLOAT32_SMALLEST_NORMAL, 0.0 ) );
    t.ok( real( v ) < -1e37, 'real component is very large and negative' );
    t.strictEqual( imag( v ), 0.0, 'imaginary component is 0' );

    t.end();
});

You can try this. I think there might be a loss of precision as we are handling single-precision floating-point numbers and as we are not explicitly returning PINF or NINF in our implementation, so, we are not comparing with exactly PINF and NINF here.

@gunjjoshi
Copy link
Member

You can declare separate variables for tiny and huge, such as:

var tiny = -1e37;
var huge = 1e37;

and then use them in the tests.

@aayush0325
Copy link
Member Author

thanks @gunjjoshi! i'll push these changes soon, anything else to add? i'll make all the changes together then.

@gunjjoshi
Copy link
Member

thanks @gunjjoshi! i'll push these changes soon, anything else to add? i'll make all the changes together then.

Nothing for now, once you've added these tests in both test.js and test.native.js, we can review it.

@aayush0325
Copy link
Member Author

updated the tests, kindly review @gunjjoshi.

@gunjjoshi gunjjoshi added the Needs Changes Pull request which needs changes before being merged. label Nov 13, 2024
@gunjjoshi
Copy link
Member

gunjjoshi commented Nov 17, 2024

The failing tests might be originating from

comment_id: botComment.id,
as we're not declaring botComment, if it is a bot comment.

cc: @Planeshifter

@aayush0325
Copy link
Member Author

I see, anything else I can do in this PR @gunjjoshi?

@aayush0325
Copy link
Member Author

The tests seem to be running fine locally for both test.js and test.native.js, not sure why there's an error here.

@aayush0325 aayush0325 requested a review from gunjjoshi November 18, 2024 02:16
@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

/stdlib merge

@kgryte
Copy link
Member

kgryte commented Nov 18, 2024

not sure why there's an error here

Test coverage build errors should be resolved now, given recent fixes by @Planeshifter to our CI.

@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Nov 18, 2024
@aayush0325
Copy link
Member Author

/stdlib merge

@stdlib-bot stdlib-bot added the bot: In Progress Pull request is currently awaiting automation. label Nov 28, 2024
@stdlib-bot stdlib-bot removed the bot: In Progress Pull request is currently awaiting automation. label Nov 28, 2024
@aayush0325
Copy link
Member Author

@gunjjoshi kindly give this a review!

Comment on lines 79 to 93
```javascript
var Complex64 = require( '@stdlib/complex/float32/ctor' );
var uniform = require( '@stdlib/random/base/uniform' );
var cinvf = require( '@stdlib/math/base/special/cinvf' );

var z1;
var z2;
var i;

for ( i = 0; i < 100; i++ ) {
z1 = new Complex64( uniform( -50.0, 50.0 ), uniform( -50.0, 50.0 ) );
z2 = cinvf( z1 );
console.log( '1.0 / (%s) = %s', z1.toString(), z2.toString() );
}
```
Copy link
Member

Choose a reason for hiding this comment

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

@aayush0325 Can you modify this example to generate the example values outside the loop? You can use @stdlib/random/array/uniform here.

Copy link
Member

Choose a reason for hiding this comment

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

You can follow this in examples/index.js as well.

@aayush0325 aayush0325 requested a review from gunjjoshi December 8, 2024 13:24
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Dec 8, 2024
Comment on lines +78 to +81
if ( ab >= LARGE_THRESHOLD ) {
re *= 0.5;
im *= 0.5;
s *= 0.5;
Copy link
Member

Choose a reason for hiding this comment

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

@aayush0325 Can you add a test both in test.js and test.native.js, to test this block? This block is not being reached in any test, according to the code coverage report.

Copy link
Member Author

@aayush0325 aayush0325 Dec 9, 2024

Choose a reason for hiding this comment

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

@gunjjoshi should i increase the range from 1e+30 to 1e+38 in all fixtures so that the code reaches this block or should i just add 4 new fixtures for very large positive/negative imaginary and very large positive/negative real (which goes to the order of +- 1e+38). we may need to further increase the tolerance of tests as well i think.

Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need to create fixtures (as in fixture files), but simply need to add a separate test block in which you hard code a few test values which exercise the branch. In that test block, you can specify a test tolerance independent of other test blocks.

Copy link
Member

Choose a reason for hiding this comment

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

I do recommend, however, validating what test values you use against a known reference implementation.

If it is easier to just create additional fixture files based on a reference implementation, that is also fine.

@aayush0325
Copy link
Member Author

aayush0325 commented Dec 11, 2024

good morning, i've added a test with hardcoded values where magnitude of the real and imaginary component exceeds LARGE_THRESHOLD so that the code reaches the mentioned block, i have taken these values from the numpy implementation using the following code:

import numpy as np

# Function to compute the inverse of a complex number using single-precision floating-point numbers
def compute_inverse_single_precision(real, imag):
    complex_number = np.complex64(real + imag * 1j)
    inverse = 1 / complex_number
    return inverse

# Given complex numbers
complex_numbers = [
    (2e+38, 200),
    (-2e+38, 200),
    (200, 2e+38),
    (200, -2e+38)
]

# Compute inverses
for real, imag in complex_numbers:
    inverse = compute_inverse_single_precision(real, imag)
    print(f"The inverse of {real} + {imag}I is {inverse}")

which gave me the output:

The inverse of 2e+38 + 200I is (4.999999675228202e-39-0j)
The inverse of -2e+38 + 200I is (-4.999999675228202e-39-0j)
The inverse of 200 + 2e+38I is -4.999999675228202e-39j
The inverse of 200 + -2e+38I is 4.999999675228202e-39j

if there's anything else that i should do then please let me know!

@aayush0325 aayush0325 requested a review from gunjjoshi December 11, 2024 08:29
@kgryte kgryte removed the Needs Review A pull request which needs code review. label Dec 14, 2024
@AadishJ
Copy link
Contributor

AadishJ commented Jan 11, 2025

hey is this implementation complete, I was trying to do this one as I did not find it in the source code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality. Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants