-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add stats/base/dists/halfnormal/pdf
#9694
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
---
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: 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
---
|
👋 Hi there! 👋 And thank you for opening your first pull request! We will review it shortly. 🏃 💨 Getting Started
Next Steps
Running Tests LocallyYou can use # Run tests for all packages in the math namespace:
make test TESTS_FILTER=".*/@stdlib/math/.*"
# Run benchmarks for a specific package:
make benchmark BENCHMARKS_FILTER=".*/@stdlib/math/base/special/sin/.*"If you haven't heard back from us within two weeks, please ping us by tagging the "reviewers" team in a comment on this PR. If you have any further questions while waiting for a response, please join our Zulip community to chat with project maintainers and other community members. We appreciate your contribution! Documentation Links |
Coverage ReportNo coverage information available. |
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 your first PR!
Please address the review comments and workflow failures.
| > y = {{alias}}( 2.0, 1.0 ) | ||
| ~0.108 | ||
| > y = {{alias}}( 0.5, 1.0 ) | ||
| ~0.352 |
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 expected value here is incorrect. pdf(0.5, 1.0) returns approximately 0.704, not 0.352.
| ~0.352 | |
| ~0.704 |
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.
Fixed in latest commit.
| > var y = myPDF( 0.0 ) | ||
| ~0.399 | ||
| > y = myPDF( 2.0 ) | ||
| ~0.192 |
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 expected value here is incorrect. myPDF(2.0) with sigma=2.0 returns approximately 0.242, not 0.192.
| ~0.192 | |
| ~0.242 |
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law of or agreed to in writing, software |
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.
There's a typo in the license header - "law of or agreed" should be "law or agreed". Curious how that happened.
| * Unless required by applicable law of or agreed to in writing, software | |
| * Unless required by applicable law or agreed to in writing, software |
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.
I’ve pushed two new commits addressing the review feedback. The first commit fixes the incorrect numeric values in the half-normal PDF documentation examples, and the second commit addresses the remaining style issues including the license header typo, indentation, and formatting as requested.
Kindly review when you have time, and please let me know if anything else needs adjustment. I’m happy to iterate further.
| if ( stdlib_base_is_nan( x ) || stdlib_base_is_nan( sigma ) || sigma <= 0.0 ) { | ||
| return 0.0/0.0; // NaN | ||
| } | ||
| if( x < 0.0 ) { |
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.
Missing space after if - should be if ( not if(
| if( x < 0.0 ) { | |
| if ( x < 0.0 ) { |
| if( x < 0.0 ) { | ||
| return 0.0; | ||
| } | ||
| const double C = x / sigma; |
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.
In C89/C90, all variable declarations must appear at the beginning of a block, before any executable statements. This declaration should be moved to the top of the function body (right after the opening brace on line 36), like:
double stdlib_base_dists_halfnormal_pdf( const double x, const double sigma ) {
double C;
if ( stdlib_base_is_nan( x ) || ...| y = pdf( -1.0 ); | ||
| t.strictEqual( isnan(y), true, 'returns expected value' ); |
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.
These lines use spaces for indentation instead of tabs. Per the stdlib style guide, JavaScript files should use TAB indentation. Also, isnan(y) should have spaces inside the parentheses: isnan( y ).
| y = pdf( -1.0 ); | |
| t.strictEqual( isnan(y), true, 'returns expected value' ); | |
| y = pdf( -1.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
| t.strictEqual( isnan(y), true, 'returns expected value' ); | ||
|
|
||
| y = pdf( PINF ); | ||
| t.strictEqual( isnan(y), true, 'returns expected value' ); |
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.
Missing spaces inside parentheses - should be isnan( y ) for consistency with the rest of the file.
| t.strictEqual( isnan(y), true, 'returns expected value' ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
| t.strictEqual( isnan(y), true, 'returns expected value' ); | ||
|
|
||
| y = pdf( NINF ); | ||
| t.strictEqual( isnan(y), true, 'returns expected value' ); |
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.
Missing spaces inside parentheses - should be isnan( y ) for consistency.
| t.strictEqual( isnan(y), true, 'returns expected value' ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
| y = pdf( -1.0, 0.0 ); | ||
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
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.
These lines use spaces for indentation instead of tabs. Per the stdlib style guide, JavaScript files should use TAB indentation.
| y = pdf( -1.0, 0.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); | |
| y = pdf( -1.0, 0.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
---
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: passed
- 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
---
---
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: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: missing_dependencies
- 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
---
stats/base/dists/halfnormal/pdf
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.
This PR should be getting close; left one more round of comments.
|
|
||
| @license Apache-2.0 | ||
|
|
||
| Copyright (c) 2018 The Stdlib Authors. |
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 a new file, so the copyright year should be 2026 rather than 2018.
| Copyright (c) 2018 The Stdlib Authors. | |
| Copyright (c) 2026 The Stdlib Authors. |
|
|
||
| [halfnormal-distribution]: https://en.wikipedia.org/wiki/Half-normal_distribution | ||
|
|
||
| [degenerate-distribution]: https://en.wikipedia.org/wiki/Degenerate_distribution |
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 link reference isn't used anywhere in the document - looks like copy-paste residue that can be removed.
| [degenerate-distribution]: https://en.wikipedia.org/wiki/Degenerate_distribution |
| /** | ||
| * @license Apache-2.0 | ||
| * | ||
| * Copyright (c) 2018 The Stdlib Authors. |
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.
New file - copyright year should be 2026.
| * Copyright (c) 2018 The Stdlib Authors. | |
| * Copyright (c) 2026 The Stdlib Authors. |
| /** | ||
| * @license Apache-2.0 | ||
| * | ||
| * Copyright (c) 2018 The Stdlib Authors. |
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.
New file - copyright year should be 2026.
| * Copyright (c) 2018 The Stdlib Authors. | |
| * Copyright (c) 2026 The Stdlib Authors. |
| * var y = myPDF( 0.5 ); | ||
| * // returns ~0.387 | ||
| */ | ||
| declare const pdf: PDF; |
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.
For consistency with other stdlib PDF packages, this should use declare var instead of declare const.
| declare const pdf: PDF; | |
| declare var pdf: PDF; |
| * @return evaluated PDF | ||
| * | ||
| * @example | ||
| * double y = stdlib_stats_base_dists_halfnormal_pdf( 2.0, 1.0 ); |
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 function name in the example doesn't match the actual function name. It says stdlib_stats_base_dists_halfnormal_pdf but should be stdlib_base_dists_halfnormal_pdf.
| * double y = stdlib_stats_base_dists_halfnormal_pdf( 2.0, 1.0 ); | |
| * double y = stdlib_base_dists_halfnormal_pdf( 2.0, 1.0 ); |
| /** | ||
| * Returns a function for evaluating the probability density function (PDF) for a half-normal distribution. | ||
| * | ||
| * @param {NonNegativeNumber} sigma - scale parameter |
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.
Since sigma <= 0.0 returns NaN (not just sigma < 0), this should be {PositiveNumber} rather than {NonNegativeNumber}.
| * @param {NonNegativeNumber} sigma - scale parameter | |
| * @param {PositiveNumber} sigma - scale parameter |
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.
I’ve pushed two new commits to address the latest review feedback.
The first commit updates the documentation in README.md (including the copyright year and factory documentation), and the second commit fixes the remaining issues across the source and type files.
Kindly take another look when you have time, and please let me know if anything else needs to be adjusted.
---
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: 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
---
---
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: missing_dependencies
- 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: passed
- task: lint_license_headers
status: passed
---
|
Hello @Planeshifter, I’ve updated the PR with the changes you requested and addressed the new comments. Could you please take another look when you have time? Let me know if everything looks good to proceed with merging. Thank you! |
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.
Did another pass.
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 file should be updated such that the code matches what's in the README.md file's ## Examples section, i.e. use logMapEach here.
| ] | ||
| } | ||
| ] | ||
| } No newline at end of file |
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.
Trailing newline missing.
| * @example | ||
| * var pdf = require( '@stdlib/stats/base/dists/halfnormal/pdf' ); | ||
| * | ||
| * var y = pdf( 2.0, 1.0 ); | ||
| * // returns ~0.108 | ||
| * | ||
| * y = pdf( 0.5, 1.0 ); | ||
| * // returns ~0.704 | ||
| * | ||
| * y = pdf( -1.0, 1.0 ); | ||
| * // returns 0.0 | ||
| */ |
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 JSDoc here is missing the factory example. Other distribution packages (like levy/pdf, rayleigh/pdf, chi/pdf) include a second @example block demonstrating pdf.factory(). For consistency, let's add one:
| * @example | |
| * var pdf = require( '@stdlib/stats/base/dists/halfnormal/pdf' ); | |
| * | |
| * var y = pdf( 2.0, 1.0 ); | |
| * // returns ~0.108 | |
| * | |
| * y = pdf( 0.5, 1.0 ); | |
| * // returns ~0.704 | |
| * | |
| * y = pdf( -1.0, 1.0 ); | |
| * // returns 0.0 | |
| */ | |
| * @example | |
| * var pdf = require( '@stdlib/stats/base/dists/halfnormal/pdf' ); | |
| * | |
| * var y = pdf( 2.0, 1.0 ); | |
| * // returns ~0.108 | |
| * | |
| * @example | |
| * var factory = require( '@stdlib/stats/base/dists/halfnormal/pdf' ).factory; | |
| * | |
| * var mypdf = factory( 1.0 ); | |
| * | |
| * var y = mypdf( 2.0 ); | |
| * // returns ~0.108 | |
| */ |
| ] | ||
| } | ||
| ] | ||
| } No newline at end of file |
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.
Missing trailing newline at end of file. Per stdlib style guide, all files must end with exactly one trailing newline character.
| x = random_uniform( 0.0, 10.0 ); | ||
| sigma = random_uniform( 0.0, 10.0 ); | ||
| y = stdlib_base_dists_halfnormal_pdf( x, sigma ); | ||
| printf( "x: %.4f, σ: %.4f, f(x;σ): %.4f\n", x, sigma, y ); |
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.
Let's use %lf for double format specifiers to match the stdlib convention (see beta/pdf/examples/c/example.c, levy/pdf/examples/c/example.c, etc.):
| printf( "x: %.4f, σ: %.4f, f(x;σ): %.4f\n", x, sigma, y ); | |
| printf( "x: %lf, σ: %lf, f(x;σ): %lf\n", x, sigma, y ); |
| var y = pdf( -1.0, 1.0 ); | ||
| t.strictEqual( y, 0.0, 'returns 0' ); |
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 here:
| var y = pdf( -1.0, 1.0 ); | |
| t.strictEqual( y, 0.0, 'returns 0' ); | |
| var y = pdf( -1.0, 1.0 ); | |
| t.strictEqual( y, 0.0, 'returns expected value' ); |
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | ||
|
|
||
| y = pdf( 0.0, NaN ); | ||
| t.strictEqual( isnan( y ), true, 'returns 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.
Same as in test.js - let's use 'returns expected value' for consistency:
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | |
| y = pdf( 0.0, NaN ); | |
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); | |
| y = pdf( 0.0, NaN ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
| y = pdf( 2.0, -1.0 ); | ||
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | ||
|
|
||
| y = pdf( 2.0, 0.0 ); | ||
| t.strictEqual( isnan( y ), true, 'returns 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.
| y = pdf( 2.0, -1.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | |
| y = pdf( 2.0, 0.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns NaN' ); | |
| y = pdf( 2.0, -1.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); | |
| y = pdf( 2.0, 0.0 ); | |
| t.strictEqual( isnan( y ), true, 'returns expected value' ); |
| var y = pdf( -1.0, 1.0 ); | ||
| t.strictEqual( y, 0.0, 'returns 0' ); |
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.
| var y = pdf( -1.0, 1.0 ); | |
| t.strictEqual( y, 0.0, 'returns 0' ); | |
| var y = pdf( -1.0, 1.0 ); | |
| t.strictEqual( y, 0.0, 'returns expected value' ); |
| } | ||
| b.pass( 'benchmark finished' ); | ||
| b.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.
Since the package exports a factory method, let's add a factory benchmark to match other distribution packages like levy/pdf. You can add something like:
bench( pkg+':factory', function benchmark( b ) {
var mypdf;
var opts;
var x;
var y;
var i;
opts = {
'dtype': 'float64'
};
mypdf = pdf.factory( 2.0 );
x = uniform( 100, 0.0, 10.0, opts );
b.tic();
for ( i = 0; i < b.iterations; i++ ) {
y = mypdf( x[ i % x.length ] );
if ( isnan( y ) ) {
b.fail( 'should not return NaN' );
}
}
b.toc();
if ( isnan( y ) ) {
b.fail( 'should not return NaN' );
}
b.pass( 'benchmark finished' );
b.end();
});---
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: 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: na
- task: lint_c_examples
status: missing_dependencies
- 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
---
|
@Planeshifter Thank you for carefully reviewing my changes. This is my first time contributing to stdlib, and I really appreciate you taking the time to point out and help me correct my mistakes. I’ve addressed your previous comments and made the requested changes please have another look and let me know if everything looks good now. |
Progresses #9416.
Description
This pull request:
Related Issues
This pull request has the following related issues:
@stdlib/stats/base/dists/halfnormalpackage #9416Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
I used AI assistance to verify the mathematical formula for the half-normal PDF and to confirm expected numerical values.
@stdlib-js/reviewers