feat: add C implementation for math/base/special/heaviside#9844
feat: add C implementation for math/base/special/heaviside#9844officiallyanee wants to merge 2 commits intostdlib-js:developfrom
math/base/special/heaviside#9844Conversation
---
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: 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: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: passed
- task: lint_c_examples
status: passed
- task: lint_c_benchmarks
status: passed
- 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/heaviside
Planeshifter
left a comment
There was a problem hiding this comment.
This PR seems somewhat confused. The title doesn't match the PR body of adding a C implementation, and a lot of the additions seem to be for a float32 variant (which would be heavisidef) instead of the float64 one. Please take a closer look at your proposed changes in addition to addressing the comments I left.
|
|
||
| // MAIN // | ||
|
|
||
| bench( format( '::native', pkg, opts ), function benchmark( b ) { |
There was a problem hiding this comment.
The benchmark name format here isn't quite right. The format() call is passing pkg and opts but the format string '::native' has no specifiers, so pkg is ignored. Also, opts needs to be the second argument to bench(), not to format().
| b.tic(); | ||
| for ( i = 0; i < b.iterations; i++ ) { | ||
| // DISCONTINUOUS | ||
| y = heaviside( x[ i%x.length ] ); |
There was a problem hiding this comment.
Missing the second argument here. Since the benchmark comment says // DISCONTINUOUS, you'll want to pass 'discontinuous' as the continuity option:
| y = heaviside( x[ i%x.length ] ); | |
| y = heaviside( x[ i%x.length ], 'discontinuous' ); |
| b.end(); | ||
| }); | ||
|
|
||
| bench( format( '::native:left-continuous', pkg, opts ), function benchmark( b ) { |
There was a problem hiding this comment.
Same benchmark name format issue here - opts needs to be the second argument to bench().
| b.tic(); | ||
| for ( i = 0; i < b.iterations; i++ ) { | ||
| // LEFT_CONTINUOUS | ||
| y = heaviside( x[ i%x.length ], 1 ); |
There was a problem hiding this comment.
The native.js wrapper expects a string continuity value that gets converted via str2enum. Passing an integer here will cause str2enum to return -1, which hits the default case in the C code and returns NaN for x == 0.
| y = heaviside( x[ i%x.length ], 1 ); | |
| y = heaviside( x[ i%x.length ], 'left-continuous' ); |
| b.end(); | ||
| }); | ||
|
|
||
| bench( format( '::native:right-continuous', pkg, opts ), function benchmark( b ) { |
There was a problem hiding this comment.
Same benchmark name format issue.
| b.tic(); | ||
| for ( i = 0; i < b.iterations; i++ ) { | ||
| // HALF_MAXIMUM | ||
| y = heaviside( x[ i%x.length ], 0 ); |
There was a problem hiding this comment.
Same issue - needs a string instead of integer:
| y = heaviside( x[ i%x.length ], 0 ); | |
| y = heaviside( x[ i%x.length ], 'half-maximum' ); |
| } | ||
| #endif | ||
|
|
||
| #endif // !STDLIB_MATH_BASE_SPECIAL_HEAVISIDEF_H |
There was a problem hiding this comment.
Small typo in the header guard comment - extra 'F' snuck in:
| #endif // !STDLIB_MATH_BASE_SPECIAL_HEAVISIDEF_H | |
| #endif // !STDLIB_MATH_BASE_SPECIAL_HEAVISIDE_H |
| /** | ||
| * @license Apache-2.0 | ||
| * | ||
| * Copyright (c) 2025 The Stdlib Authors. |
There was a problem hiding this comment.
Copyright year should be 2026 for new files:
| * Copyright (c) 2025 The Stdlib Authors. | |
| * Copyright (c) 2026 The Stdlib Authors. |
| * @return function value | ||
| * | ||
| * @example | ||
| * double y = stdlib_base_heaviside( -9.0 ); |
There was a problem hiding this comment.
The example is missing the second parameter. The function requires both x and continuity:
| * double y = stdlib_base_heaviside( -9.0 ); | |
| * double y = stdlib_base_heaviside( -9.0, STDLIB_BASE_HEAVISIDE_CONTINUITY_HALF_MAXIMUM ); |
| Evaluates the [Heaviside function][heaviside-function]. | ||
|
|
||
| ```c | ||
| float y = stdlib_base_heaviside( 0.0, STDLIB_BASE_HEAVISIDE_CONTINUITY_HALF_MAXIMUM ); |
There was a problem hiding this comment.
The function returns double, not float. Let's fix the variable type:
| float y = stdlib_base_heaviside( 0.0, STDLIB_BASE_HEAVISIDE_CONTINUITY_HALF_MAXIMUM ); | |
| double y = stdlib_base_heaviside( 0.0, STDLIB_BASE_HEAVISIDE_CONTINUITY_HALF_MAXIMUM ); |
…rameter passing & typo in examples
---
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: 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: na
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: passed
- 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
---
math/base/special/heavisidemath/base/special/heaviside
|
@Planeshifter thank you for the review! I have done the changes you mentioned , but i did not find any float32 additions other than the ones you mentioned. |
|
@officiallyanee Is there a reason why you chose to open this PR when we already have
? |
I apologize, there were a lot of PRs under this issue majority of which were merged if they were not newer PRs. I didn't see code for it in the develop branch and none of the recent PRs addressed it so I went ahead with it. |
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves none.
Description
This pull request:
math/base/special/heavisideRelated 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
{{TODO: add disclosure if applicable}}
@stdlib-js/reviewers