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 24 commits into
base: develop
Choose a base branch
from

Conversation

aayush0325
Copy link
Contributor

@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
Contributor 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
Contributor 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
Contributor Author

updated the tests, kindly review @gunjjoshi.

lib/node_modules/@stdlib/math/base/special/cinvf/README.md Outdated Show resolved Hide resolved
lib/node_modules/@stdlib/math/base/special/cinvf/README.md Outdated Show resolved Hide resolved
t.strictEqual( real( q ), qre[ i ], 'returns expected real component' );
} else {
delta = absf( real( q ) - qre[ i ] );
tol = 5 * EPS * absf( qre[ i ] );
Copy link
Member

Choose a reason for hiding this comment

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

This tolerance, and tolerances for the tests below are different from what we have in test.js. Is there any specific reason for this? Ideally, the tolerances much match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when i run the tests locally, the tolerance in test.js was appropriate for all test cases to pass. the c files had some more inaccuracy due to the single-precision nature of the code hence i had to increase the tolerance to pass the tests. i think this is because i used fmaxf instead of stdlib_base_maxf. will update the C code and see if i can match both the tolerances.

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

aayush0325 commented Nov 13, 2024

@gunjjoshi even after using stdlib_base_maxf from math/base/special/maxf the tests are failing for C files. i think we should try to write runner.jl to have more accurate data for single-precision floating-point numbers and then see if the same tolerance is working. how should i proceed with this?

@gunjjoshi
Copy link
Member

gunjjoshi commented Nov 13, 2024

@aayush0325 As mentioned above, you should make sure that the fixtures generated fall within the range of single-precision floating-point numbers.
For example, compare the range of fixtures in math/base/special/ln, with that in its single-precision equivalent, i.e., math/base/special/lnf. Notice that we scale down from ~308 to ~38, when we go from double to single-precision.
Something similar needs to be followed here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants