-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add implementation for stats/base/dists/halfnormal/quantile
#9723
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: 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
---
---
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: 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
---
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.
Looks close to be ready.
| ) { | ||
| return 0.0 / 0.0; // NaN | ||
| } | ||
| if( sigma == 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 and before opening brace. stdlib C style requires if ( condition ) {.
| if( sigma == 0.0 ){ | |
| if ( sigma == 0.0 ) { |
| if( sigma == 0.0 ){ | ||
| return 0.0; | ||
| } | ||
| if( p == 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.
Same here - needs spaces after if and before the brace.
| if( p == 1.0 ){ | |
| if ( p == 1.0 ) { |
|
|
||
| /** | ||
| * Compute the variance of a one-dimensional ndarray using Welford's algorithm. | ||
| * Halfnormal distribution quantile function. |
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 hyphen - should be "Half-normal" (with hyphen) to match the README and other documentation in this package.
| * Halfnormal distribution quantile function. | |
| * Half-normal distribution quantile function. |
Coverage Report
The above coverage report was generated for the changes in this PR. |
---
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: 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
---
|
Thank you @Planeshifter for reviewing my PR. I’ve addressed the style issue in main.c and ensured that Half-normal is used consistently as the name throughout the package. Please let me know if anything else needs to be updated. |
---
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
---
| b.end(); | ||
| }); | ||
|
|
||
| bench( pkg+':factory', function benchmark( b ) { |
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.
| bench( pkg+':factory', function benchmark( b ) { | |
| bench( format( '%s::factory', pkg ), function benchmark( b ) { |
| var bench = require( '@stdlib/bench' ); | ||
| var Float64Array = require( '@stdlib/array/float64' ); | ||
| var uniform = require( '@stdlib/random/base/uniform' ); | ||
| var isnan = 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.
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | |
| var isnan = require( '@stdlib/math/base/assert/is-nan' ); | |
| var format = require( '@stdlib/string/format' ); |
| } | ||
| return quantile; |
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.
Why is sigma not being precomputed through factory fcn here
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 a Half-normal distribution, sigma is CONSTANT for a given factory instance, but the quantile value itself depends on the input probability p. As a result, there isn’t anything meaningful to precompute inside factory.js beyond validating sigma and handling edge cases which is correctly being done here. TLDR nothing to precompute. However a little optimization of calculating sigma * sqrt(2) can be done and i will fix it.
Neerajpathak07
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.
@Om-A-osc This PR will need a bit off refactoring. I suggest you to take a closer look at how the statistical distribution packages are implemented w.r.t the current code convention.
A few examples that I have in mind are:-
wald/pdfwald/meanhalfnormal/stdev
| ``` | ||
|
|
||
| <!-- <div class="equation" align="center" data-raw-text="Q(p;\sigma) = \sigma\sqrt{2}\,\operatorname{erf}^{-1}(p)" data-equation="eq:halfnormal_quantile_function"> | ||
| <img src="https://cdn.jsdelivr.net/gh/stdlib-js/stdlib@bb29798d6de5a1de3c633e038f832c3c04207865/lib/node_modules/@stdlib/stats/base/dists/halfnormal/quantile/docs/img/equation_halfnormal_quantile_function.svg" alt="Quantile function for a Half-normal 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.
Better to remove this link as you haven't added the svg file for halfnormal/quantile
|
|
||
| #### quantile.factory( sigma ) | ||
|
|
||
| Returns a function for evaluating the [quantile function][quantile-function] of a [Half-normal][halfnormal-distribution] distribution with parameter `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.
| Returns a function for evaluating the [quantile function][quantile-function] of a [Half-normal][halfnormal-distribution] distribution with parameter `sigma`. | |
| Returns a function for evaluating the [quantile function][quantile-function] of a [Half-normal][halfnormal-distribution] distribution with scale parameter `sigma`. |
| len = 100; | ||
| p = new Float64Array( len ); | ||
| sigma = new Float64Array( len ); | ||
| for ( i = 0; i < len; i++ ) { | ||
| p[ i ] = uniform( 0.0, 1.0 ); | ||
| sigma[ i ] = uniform( EPS, 20.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.
| len = 100; | |
| p = new Float64Array( len ); | |
| sigma = new Float64Array( len ); | |
| for ( i = 0; i < len; i++ ) { | |
| p[ i ] = uniform( 0.0, 1.0 ); | |
| sigma[ i ] = uniform( EPS, 20.0 ); | |
| } | |
| len = 100; | |
| p = uniform( len, 0.0, 1.0 ); | |
| sigma = uniform( len, EPS, 20.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.
If you’re referring specifically to @stdlib/random/base/uniform, then it’s worth checking the actual implementation before drawing conclusions.
At the core level, the arguments are not interpreted as (length, min, max). The uniform generator is scalar, and its signature is:
* Returns a uniformly distributed pseudorandom number with minimum support `a`
* and maximum support `b`.
*
* @private
* @param {PRNG} rand - pseudorandom number generator
* @param {number} a - minimum support (inclusive)
* @param {number} b - maximum support (exclusive)
* @returns {number} pseudorandom number
*/
function uniform( rand, a, b ) {
var r = rand();
return ( b*r ) + ( (1.0-r)*a ); // equivalent to (b-a)*r + a
}Here:
aandbare bounds- the first argument is a PRNG function
- there is no notion of array size at this level
Calling something like uniform(100, 0, 1) would be invalid for this function.
100 would be treated as a PRNG and and will give incorrect result.
The current benchmark is correct and passes all benchmark tests and should be good.
If the comment is about a different package or wrapper, could you please mention its name explicitly so we’re discussing the same API? That would help keep the feedback focused and actionable.
Please review the implementation and call logic before commenting, and limit
suggestions to concrete, credible improvements rather than assumptions.
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.
Here we are using:-
var uniform = require( '@stdlib/random/array/uniform' );And the reason for this modification is to remove the need of for loop and generate values up to a len of 100 for both variables. This is also present in a few of the upstream statistical packages which is why I recommended you to take a look at how it was implemented there.
You're right I should have added a comment suggesting to update the path as well. Sorry for the confusion.
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.
@Neerajpathak07 is correct. We have moved away from generating individual random variates toward preallocating arrays.
| sigma = 4.0; | ||
| myquantile = quantile.factory( sigma ); | ||
| len = 100; | ||
| p = new Float64Array( len ); | ||
| for ( i = 0; i < len; i++ ) { | ||
| p[ i ] = uniform( 0.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.
| sigma = 4.0; | |
| myquantile = quantile.factory( sigma ); | |
| len = 100; | |
| p = new Float64Array( len ); | |
| for ( i = 0; i < len; i++ ) { | |
| p[ i ] = uniform( 0.0, 1.0 ); | |
| } | |
| myquantile = quantile.factory( 4.0 ); | |
| p = uniform( 0.0, 1.0 ); |
| len = 100; | ||
| p = new Float64Array( len ); | ||
| sigma = new Float64Array( len ); | ||
| for ( i = 0; i < len; i++ ) { | ||
| p[ i ] = uniform( 0.0, 1.0 ); | ||
| sigma[ i ] = uniform( EPS, 20.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 comment
lib/node_modules/@stdlib/stats/base/dists/halfnormal/quantile/src/main.c
Show resolved
Hide resolved
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.
Why is this file empty.
We are expected to add dependencies to generate test fixtures here.
| for i in eachindex(p) | ||
| # Half-normal quantile: | ||
| # Q(p) = sigma * sqrt(2) * erfinv(p) | ||
| z[i] = sigma[i] * sqrt(2.0) * erfinv(p[i]) |
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.
better to import halfnormal form the distributions and use here.
ref:- https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/stats/base/dists/halfnormal/stdev/test/fixtures/julia/runner.jl
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.
HalfNormal doesn't exist in Julia; that was an oversight that didn't get caught in PR review. We still need to clean up the runner in the stdev package.
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.
Yeah just went through julia's doc to see if it supports halfnormal turns out it doesn't.
@Planeshifter but I'm curious how was it able to generate the fixtures in the first place?
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 fixtures must have been generated in some other way...
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.
Use SciPy.
| } else { | ||
| delta = abs( q( p[i] ) - expected[i] ); | ||
| tol = 40.0 * EPS * abs( expected[i] ); | ||
| t.ok(delta <= tol, 'p: '+p[i]+', sigma: '+sigma[i]+', q: '+q(p[i])+', expected: '+expected[i]); | ||
| } |
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.
We can modify this test to use isAlmostSameValue var
ref:-
| t.ok( isAlmostSameValue( y, expected[i], 1500 ), 'within tolerance. x: '+x[ i ]+'. mu: '+mu[i]+'. lambda: '+lambda[i]+'. y: '+y+'. E: '+expected[ i ]+'.' ); |
Also update the other test files with this!
| var i; | ||
|
|
||
| for ( i = 0; i < p.length; i++ ) { | ||
| y = quantile( p[i], sigma[i] ); |
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 am a bit confused on why the values for p and sigma in this edge case are not coming form the generated fixture which is data.json
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.
Please take a closer look p and sigma are indeed coming from data.json, as shown by their declarations at the top of the 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.
Yes but at the incorrect position instead of L34 we need to add it inside this edge case. Do you know why we are doing this?
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 have made the change in latest commit. I am not sure but ig its better to have data in test stack rather than heap .Would love to learn more.
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.
Yeah almost there so in your previous attainment you had drawn data from the fixture correctly but it was placed inside the // fixture def were we defined where the auto-generated fixtures are coming from. In-order to keep that def clean and meaningful we avoid any implicit changes.
Also a fact that loading generated values inside a test case when needed can be much more viable and readable.
This is also something that I learned through time, so no need to beat yourself to it!!!!
---
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: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: missing_dependencies
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- 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: 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: na
- task: lint_javascript_benchmarks
status: na
- task: lint_python
status: missing_dependencies
- 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
---
…tfile comments
---
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: passed
- task: lint_python
status: passed
- 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
---
|
@Neerajpathak07 @Planeshifter Summary of changes:
All tests, linters, and benchmarks (JS + native) are now passing . Thank you for taking the time to review this and for the helpful feedback. Please have a look when you get a chance I’m happy to make any further adjustments if needed. |
| }); | ||
|
|
||
| tape( 'if any argument is NaN, the function returns NaN', function test( t ) { | ||
| t.ok( isnan( quantile( NaN, 1.0 ) ), 'p 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.
You're deviating from a number of our test conventions here. Typically, we use an explicit t.strictEqual. Furthermore, we have standardized around always using returns expected value for the test message here and below.
There are reasons for this. Namely, to assist in future migration to an in-house test runner. If everything adheres to common conventions, this will make migration much more straightforward.
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.
We cannot use strictEqual for testing NaN values herer, since in JavaScript NaN === NaN evaluates to false. However, strictEqual will still be used wherever possible, and I will make sure we use "returns expected value" the test message.
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'the function evaluates the Half-normal quantile correctly', function test( 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.
| tape( 'the function evaluates the Half-normal quantile correctly', function test( t ) { | |
| tape( 'the function evaluates the half-normal quantile', function test( t ) { |
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'p = 1 returns +Infinity', function test( 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.
| tape( 'p = 1 returns +Infinity', function test( t ) { | |
| tape( 'if p = 1, the function returns positive infinity', function test( t ) { |
| if ( expected[i] !== null ) { | ||
| if ( y === expected[i] ) { | ||
| t.strictEqual( y, expected[i], 'p: '+p[i]+', sigma: '+sigma[i]+', y: '+y+', expected: '+expected[i] ); | ||
| } else { | ||
| t.ok( isAlmostSameValue( y, expected[ i ], 40.0 ), 'within tolerance. p: '+p[ i ]+'. sigma: '+sigma[ i ]+'. y: '+y+'. expected: '+expected[ i ]+'.' ); | ||
| } | ||
| } |
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.
Consolidate to t.strictEqual( isAlmostSameValue( y, expected[ i ], 40 ), true, ... );. No need for the various branching here.
---
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: 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
---
|
@kgryte Thank you for your review. I did couple of requested changes. Have a look whenever free. |
Progressess #9416.
Description
This pull request adds a complete implementation of the Half-Normal distribution quantile function (
halfnormal/quantile) across the JavaScript, native C, and N-API layers.Related Issues
This pull request has the following related issues:
@stdlib/stats/base/dists/halfnormalpackage #9416Questions
No.
Other
All tests, benchmarks, and examples pass for both JavaScript and C implementations. The implementation handles edge cases correctly, including invalid probabilities, non-positive scale parameters, the degenerate case
σ = 0, andp = 1returning+Infinity.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
I used ChatGPT to help derive and verify the half-normal quantile formula.
@stdlib-js/reviewers