-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add math/base/special/sincosdf
#9912
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Bhargav Dabhade <bhargava2005dabhade@gmail.com>
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: passed
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: missing_dependencies
- task: lint_c_examples
status: missing_dependencies
- task: lint_c_benchmarks
status: missing_dependencies
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Signed-off-by: Bhargav Dabhade <bhargava2005dabhade@gmail.com>
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: na
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
math/base/special/sincosdf
Planeshifter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this PR!
| tape( 'the function computes the sine and cosine (for -256*180.0 < x < 0)', function test( t ) { | ||
| var cosine; | ||
| var delta; | ||
| var sine; | ||
| var tol; | ||
| var x; | ||
| var y; | ||
| var i; | ||
| var z; | ||
|
|
||
| z = [ 0.0, 0.0 ]; | ||
| x = mediumNegative.x; | ||
| sine = mediumNegative.sine; | ||
| cosine = mediumNegative.cosine; | ||
|
|
||
| for ( i = 0; i < x.length; i++ ) { | ||
| x[ i ] = f32( x[ i ] ); | ||
| y = sincosdf( x[i], z, 1, 0 ); | ||
| sine[ i ] = f32( sine[ i ] ); | ||
| cosine[ i ] = f32( cosine[ i ] ); | ||
| t.strictEqual( y, z, 'returns output array' ); | ||
| if ( y[0] === sine[ i ] ) { | ||
| t.strictEqual( y[0], sine[ i ], 'x: '+x[i]+'. Expected: '+sine[i] ); | ||
| } else { | ||
| delta = absf( y[0] - sine[i] ); | ||
| tol = EPS * absf( sine[i] ); | ||
| t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+y[0]+'. Expected: '+sine[i]+'. tol: '+tol+'. delta: '+delta+'.' ); | ||
| } | ||
| if ( y[1] === cosine[ i ] ) { | ||
| t.strictEqual( y[1], cosine[ i ], 'x: '+x[i]+'. Expected: '+cosine[i] ); | ||
| } else { | ||
| delta = absf( y[1] - cosine[i] ); | ||
| tol = 1.01 * EPS * absf( cosine[i] ); | ||
| t.ok( delta <= tol, 'within tolerance. x: '+x[i]+'. Value: '+y[1]+'. Expected: '+cosine[i]+'. tol: '+tol+'. delta: '+delta+'.' ); | ||
| } | ||
| } | ||
| t.end(); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block (lines 87-124) is an exact duplicate of the test block at lines 48-84 — same description, same mediumNegative fixture data. Looks like a copy-paste leftover. Since mediumPositive is already tested at line 126, this block should just be removed.
|
|
||
| var resolve = require( 'path' ).resolve; | ||
| var tape = require( 'tape' ); | ||
| var isnanf = require( '@stdlib/math/base/assert/is-nan' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This imports the double-precision is-nan but names it isnanf. Since this is a single-precision package, let's use the single-precision variant to stay consistent with test.main.js and test.assign.js:
| var isnanf = require( '@stdlib/math/base/assert/is-nan' ); | |
| var isnanf = require( '@stdlib/math/base/assert/is-nanf' ); |
| var PINF = require( '@stdlib/constants/float32/pinf' ); | ||
| var NINF = require( '@stdlib/constants/float32/ninf' ); | ||
| var EPS = require( '@stdlib/constants/float32/eps' ); | ||
| var absf = require( '@stdlib/math/base/special/abs' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above — this imports double-precision abs but the variable is named absf. Let's match the other test files:
| var absf = require( '@stdlib/math/base/special/abs' ); | |
| var absf = require( '@stdlib/math/base/special/absf' ); |
| * | ||
| * @private | ||
| * @param {number} x - input value (in degrees) | ||
| * @returns {Array<number>} sine and cosine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return type here should be Float32Array to match what the function actually returns. See the reference sincosf/lib/native.js for the expected pattern:
| * @returns {Array<number>} sine and cosine | |
| * @returns {Float32Array} sine and cosine |
|
|
||
| interface SinCosdf { | ||
| /** | ||
| * Simultaneously computes the sine and cosine of an angle measured in degrees (single-precision). | ||
| * | ||
| * @param x - input value (in degrees) | ||
| * @returns sine and cosine | ||
| * | ||
| * @example | ||
| * var v = sincosdf( 0.0 ); | ||
| * // returns [ ~0.0, ~1.0 ] | ||
| * | ||
| * @example | ||
| * var v = sincosdf( 90.0 ); | ||
| * // returns [ ~1.0, ~0.0 ] | ||
| * | ||
| * @example | ||
| * var v = sincosdf( -30.0 ); | ||
| * // returns [ ~-0.5, ~0.866 ] | ||
| * | ||
| * @example | ||
| * var v = sincosdf( NaN ); | ||
| * // returns [ NaN, NaN ] | ||
| */ | ||
| ( x: number ): Array<number>; | ||
|
|
||
| /** | ||
| * Simultaneously computes the sine and cosine of an angle measured in degrees (single-precision) and assigns the results to a provided output array. | ||
| * | ||
| * @param x - input value (in degrees) | ||
| * @param out - output array | ||
| * @param stride - output array stride | ||
| * @param offset - output array index offset | ||
| * @returns sine and cosine | ||
| * | ||
| * @example | ||
| * var Float32Array = require( '@stdlib/array/float32' ); | ||
| * | ||
| * var out = new Float32Array( 2 ); | ||
| * | ||
| * var v = sincosdf.assign( 0.0, out, 1, 0 ); | ||
| * // returns <Float32Array>[ ~0.0, ~1.0 ] | ||
| * | ||
| * var bool = ( v === out ); | ||
| * // returns true | ||
| */ | ||
| assign<T extends ArrayLike<number>>( ...args: [ number, T, number, number ] ): T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing the NumericArray import and the assign method signature doesn't match the stdlib pattern. Comparing with sincosf/docs/types/index.d.ts, there should be:
- An import on line 23:
import { NumericArray } from '@stdlib/types/array'; - The
assignsignature should use named parameters withNumericArray:assign<T extends NumericArray>( x: number, out: T, stride: number, offset: number ): T;
The current spread args style (...args: [ number, T, number, number ]) differs from the established pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Planeshifter Thank you for the guidance! I’ve followed your instructions and updated the code to use the named-parameters pattern with NumericArray.
After this change, I’m encountering TypeScript linting errors in the test file (lines 70–74), where invalid out arguments (number, boolean, null, object) are expected to be rejected but are not being caught by the type checker.
To double-check, I ran the linter on sincosf, which uses the identical assign(...) signature, and observed the same linting errors there as well.
This makes me think the behavior may be related to how the generic NumericArray constraint is handled by the current TypeScript version, rather than something specific to sincosdf.
Could you please advise how you’d like to proceed here to stay consistent with stdlib patterns?
| * @example | ||
| * var sincosdf = require( '@stdlib/math/base/special/sincosdf' ); | ||
| * | ||
| * var out = new Float32Array( 2 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second @example block uses new Float32Array( 2 ) but doesn't include the Float32Array import. Looking at sincosf/lib/index.js, the second example includes the import:
* var Float32Array = require( '@stdlib/array/float32' );
* var sincosdf = require( '@stdlib/math/base/special/sincosdf' );Let's add the missing import line so the example is self-contained and runnable.
Signed-off-by: Bhargav Dabhade <bhargava2005dabhade@gmail.com>
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: na
- task: lint_package_json
status: na
- task: lint_repl_help
status: na
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: na
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: na
- task: lint_license_headers
status: passed
---
Signed-off-by: Bhargav Dabhade bhargava2005dabhade@gmail.com
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves None
Description
This pull request:
Related Issues
This pull request has the following related issues:
Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
@stdlib-js/reviewers