feat: add stats/strided/snanmeankbn - #14175
Conversation
Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
|
👋 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 |
Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
This fixes an issue where the native test cases were asserting invalid return values instead of expecting NaNs to be properly ignored, aligning them with the pure JS tests so all tests pass in the C-build. Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
0PrashantYadav0
left a comment
There was a problem hiding this comment.
Make all the changes I have suggested.
| */ | ||
| interface Routine { | ||
| /** | ||
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm. |
There was a problem hiding this comment.
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm. | |
| * Computes the arithmetic mean of a single-precision floating-point strided array, ignoring `NaN` values and using an improved Kahan–Babuška algorithm. |
| * @example | ||
| * var Float32Array = require( '@stdlib/array/float32' ); | ||
| * | ||
| * var x = new Float32Array( [ 1.0, -2.0, 2.0 ] ); |
There was a problem hiding this comment.
| * var x = new Float32Array( [ 1.0, -2.0, 2.0 ] ); | |
| * var x = new Float32Array( [ 1.0, -2.0, NaN, 2.0 ] ); |
| ( N: number, x: Float32Array, strideX: number ): number; | ||
|
|
||
| /** | ||
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm and alternative indexing semantics. |
There was a problem hiding this comment.
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm and alternative indexing semantics. | |
| * Computes the arithmetic mean of a single-precision floating-point strided array, ignoring `NaN` values and using an improved Kahan–Babuška algorithm and alternative indexing semantics. |
| * @example | ||
| * var Float32Array = require( '@stdlib/array/float32' ); | ||
| * | ||
| * var x = new Float32Array( [ 1.0, -2.0, 2.0 ] ); |
There was a problem hiding this comment.
| * var x = new Float32Array( [ 1.0, -2.0, 2.0 ] ); | |
| * var x = new Float32Array( [ 1.0, -2.0, NaN, 2.0 ] ); |
| } | ||
|
|
||
| /** | ||
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm. |
There was a problem hiding this comment.
| * Computes the arithmetic mean of a single-precision floating-point strided array using an improved Kahan–Babuška algorithm. | |
| * Computes the arithmetic mean of a single-precision floating-point strided array, ignoring `NaN` values and using an improved Kahan–Babuška algorithm. |
| ## See Also | ||
|
|
||
| - <span class="package-name">[`@stdlib/stats/strided/dmeankbn`][@stdlib/stats/strided/dmeankbn]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a double-precision floating-point strided array using an improved Kahan–Babuška algorithm.</span> | ||
| - <span class="package-name">[`@stdlib/stats/strided/meankbn`][@stdlib/stats/strided/meankbn]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a strided array using an improved Kahan–Babuška algorithm.</span> |
There was a problem hiding this comment.
| - <span class="package-name">[`@stdlib/stats/strided/meankbn`][@stdlib/stats/strided/meankbn]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a strided array using an improved Kahan–Babuška algorithm.</span> | |
| - <span class="package-name">[`@stdlib/stats/strided/snanmean`][@stdlib/stats/strided/snanmean]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a single-precision floating-point strided array, ignoring NaN values.</span> |
|
|
||
| - <span class="package-name">[`@stdlib/stats/strided/dmeankbn`][@stdlib/stats/strided/dmeankbn]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a double-precision floating-point strided array using an improved Kahan–Babuška algorithm.</span> | ||
| - <span class="package-name">[`@stdlib/stats/strided/meankbn`][@stdlib/stats/strided/meankbn]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a strided array using an improved Kahan–Babuška algorithm.</span> | ||
| - <span class="package-name">[`@stdlib/stats/strided/smean`][@stdlib/stats/strided/smean]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a single-precision floating-point strided array.</span> |
There was a problem hiding this comment.
| - <span class="package-name">[`@stdlib/stats/strided/smean`][@stdlib/stats/strided/smean]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a single-precision floating-point strided array.</span> | |
| - <span class="package-name">[`@stdlib/stats/strided/snanmeanors`][@stdlib/stats/strided/snanmeanors]</span><span class="delimiter">: </span><span class="description">calculate the arithmetic mean of a single-precision floating-point strided array, ignoring NaN values and using ordinary recursive summation.</span> |
|
|
||
| [@stdlib/stats/strided/dmeankbn]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/dmeankbn | ||
|
|
||
| [@stdlib/stats/strided/meankbn]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/meankbn |
There was a problem hiding this comment.
| [@stdlib/stats/strided/meankbn]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/meankbn | |
| [@stdlib/stats/strided/smeankbn]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/smeankbn |
|
|
||
| [@stdlib/stats/strided/meankbn]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/meankbn | ||
|
|
||
| [@stdlib/stats/strided/smean]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/smean |
There was a problem hiding this comment.
| [@stdlib/stats/strided/smean]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/smean | |
| [@stdlib/stats/strided/snanmean]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/snanmean | |
| [@stdlib/stats/strided/snanmeanors]: https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/stats/strided/snanmeanors |
0PrashantYadav0
left a comment
There was a problem hiding this comment.
Make all the changes I have suggested.
1e3a746 to
34da390
Compare
|
Thanks for the detailed review @0PrashantYadav0 ! I've addressed all the review feedback across the files, updated the style formatting, and resolved the CI check failures. Local tests and linters are passing cleanly now. It's ready for another look when you have a moment! |
Coverage Report
The above coverage report was generated for the changes in this PR. |
- Update C function signatures and parameter naming according to project conventions - Align header includes and standard type definitions - Fix linting errors and formatting inconsistencies across strided math implementation - Resolve build and test runner CI failures Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
34da390 to
421cb9b
Compare
|
Hey! @AryanSharma48 Please take another look at the review—it seems like a couple of the suggested changes slipped through. (I find it helpful to resolve the comments one by one as I apply them so I don't lose track). Also, let's avoid force-pushing. Before you start working on the remaining changes, make sure to pull/fetch the latest updates from your branch. Let me know if you run into any issues! |
Remove the double-precision dmeankbn package from the See Also section, leaving only single-precision package references as requested in PR review. Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
|
Thanks for pointing that out @0PrashantYadav0! Regarding the force-push: I ran into a branch sync issue after missing a local pull, so I force-pushed to align with develop, apologies for the extra noise, I'll stick to standard incremental commits going forward! As for the reviews, a few of the suggestions were overlapping with changes already applied, but I went back through all open threads one-by-one, addressed the remaining ones, and resolved them on GitHub. I’ve pushed a fresh commit with those final fixes and local tests are passing cleanly. Ready for your review! |
Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
0PrashantYadav0
left a comment
There was a problem hiding this comment.
Some of the changes required little more discussion.
| double elapsed; | ||
| float *x; | ||
| float v; | ||
| double t; |
There was a problem hiding this comment.
Move float v; below double t;
| double elapsed; | ||
| float *x; | ||
| float v; | ||
| double t; |
There was a problem hiding this comment.
move double t; below double elapsed;
|
|
||
| // FUNCTIONS // | ||
|
|
||
| /** |
There was a problem hiding this comment.
| /** | |
| /** | |
| * Returns a random number. | |
| * | |
| * @private | |
| * @returns {number} random number or `NaN` | |
| */ | |
| function rand() { | |
| if ( bernoulli( 0.8 ) < 1 ) { | |
| return NaN; | |
| } | |
| return uniform( -10.0, 10.0 ); | |
| } | |
| /** |
| // MODULES // | ||
|
|
||
| var bench = require( '@stdlib/bench' ); | ||
| var uniform = require( '@stdlib/random/array/uniform' ); |
There was a problem hiding this comment.
| var uniform = require( '@stdlib/random/array/uniform' ); | |
| var uniform = require( '@stdlib/random/base/uniform' ); | |
| var bernoulli = require( '@stdlib/random/base/bernoulli' ); | |
| var filledarrayBy = require( '@stdlib/array/filled-by' ); |
|
|
||
| var options = { | ||
| 'dtype': 'float32' | ||
| }; |
There was a problem hiding this comment.
Remove the complete variables part.
|
|
||
| x = (float *)malloc( len * sizeof( float ) ); | ||
| for ( i = 0; i < len; i++ ) { | ||
| x[ i ] = ( rand_float() * 20000.0f ) - 10000.0f; |
There was a problem hiding this comment.
| x[ i ] = ( rand_float() * 20000.0f ) - 10000.0f; | |
| if ( rand_float() < 0.2f ) { | |
| x[ i ] = 0.0f / 0.0f; // NaN | |
| } else { | |
| x[ i ] = ( rand_float()*20000.0f ) - 10000.0f; | |
| } |
| if ( n === 0 ) { | ||
| return NaN; | ||
| } | ||
| return float64ToFloat32( (sum + c) / n ); |
There was a problem hiding this comment.
Instead of creating the complete logic from scratch we can use already existing modules. For example here we can use @stdlib/blas/ext/base/snansumkbn.
You can replace this file with:
'use strict';
// MODULES //
var float64ToFloat32 = require( '@stdlib/number/float64/base/to-float32' );
var snansumkbn = require( '@stdlib/blas/ext/base/snansumkbn' ).ndarray;
// MAIN //
/**
* Computes the arithmetic mean of a single-precision floating-point strided array, ignoring `NaN` values and using an improved Kahan–Babuška algorithm.
*
* ## Method
*
* - This implementation uses an "improved Kahan–Babuška algorithm", as described by Neumaier (1974).
*
* ## References
*
* - Neumaier, Arnold. 1974. "Rounding Error Analysis of Some Methods for Summing Finite Sums." _Zeitschrift Für Angewandte Mathematik Und Mechanik_ 54 (1): 39–51. doi:[10.1002/zamm.19740540106](https://doi.org/10.1002/zamm.19740540106).
*
* @param {PositiveInteger} N - number of indexed elements
* @param {Float32Array} x - input array
* @param {integer} strideX - stride length
* @param {NonNegativeInteger} offsetX - starting index
* @returns {number} arithmetic mean
*
* @example
* var Float32Array = require( '@stdlib/array/float32' );
*
* var x = new Float32Array( [ 2.0, 1.0, 2.0, -2.0, -2.0, 2.0, 3.0, 4.0, NaN, NaN ] );
*
* var v = snanmeankbn( 5, x, 2, 1 );
* // returns 1.25
*/
function snanmeankbn( N, x, strideX, offsetX ) {
var ix;
var v;
var n;
var i;
if ( N <= 0 ) {
return NaN;
}
if ( N === 1 || strideX === 0 ) {
return x[ offsetX ];
}
ix = offsetX;
n = 0;
for ( i = 0; i < N; i++ ) {
v = x[ ix ];
if ( v === v ) {
n += 1;
}
ix += strideX;
}
if ( n === 0 ) {
return NaN;
}
return float64ToFloat32( snansumkbn( N, x, strideX, offsetX ) / n );
}
// EXPORTS //
module.exports = snanmeankbn;Still we have to discuss this further.
| * @param offsetX starting index | ||
| * @return output value | ||
| */ | ||
| float API_SUFFIX(stdlib_strided_snanmeankbn_ndarray)( const CBLAS_INT N, const float *X, const CBLAS_INT strideX, const CBLAS_INT offsetX ) { |
There was a problem hiding this comment.
You can changes this as :
float API_SUFFIX(stdlib_strided_snanmeankbn_ndarray)( const CBLAS_INT N, const float *X, const CBLAS_INT strideX, const CBLAS_INT offsetX ) {
CBLAS_INT ix;
CBLAS_INT i;
CBLAS_INT n;
float v;
if ( N <= 0 ) {
return 0.0f / 0.0f; // NaN
}
if ( N == 1 || strideX == 0 ) {
return X[ offsetX ];
}
ix = offsetX;
n = 0;
for ( i = 0; i < N; i++ ) {
v = X[ ix ];
if ( v == v ) {
n += 1;
}
ix += strideX;
}
if ( n == 0 ) {
return 0.0f / 0.0f; // NaN
}
return (double)API_SUFFIX(stdlib_strided_snansumkbn_ndarray)( N, X, strideX, offsetX ) / (double)n;
}| #include "stdlib/stats/strided/snanmeankbn.h" | ||
| #include "stdlib/blas/base/shared.h" | ||
| #include "stdlib/strided/base/stride2offset.h" | ||
| #include <math.h> |
There was a problem hiding this comment.
Chnage the complete import as well.
#include "stdlib/stats/strided/snanmeankbn.h"
#include "stdlib/blas/ext/base/snansumkbn.h"
#include "stdlib/blas/base/shared.h"
#include "stdlib/strided/base/stride2offset.h"| "libpath": [], | ||
| "dependencies": [ | ||
| "@stdlib/blas/base/shared", | ||
| "@stdlib/strided/base/stride2offset" |
There was a problem hiding this comment.
If we move forward with my approach we make to changes the dependencies here as well.
| "@stdlib/strided/base/stride2offset" | |
| "@stdlib/strided/base/stride2offset", | |
| "@stdlib/blas/ext/base/snansumkbn" |
There was a problem hiding this comment.
Have to make changes in all the dependencies.
Resolves #13955
Description
This pull request:
@stdlib/stats/strided/snanmeankbn, implementing single-precision NaN-ignoring strided mean calculation using Kahan-Babuška-Neumaier (KBN) compensated summation.src/main.candlib/ndarray.jsto perform single-pass sum computation while dynamically filteringNaNvalues and tracking valid count (NaNwhenNaNorndarray, and negative stride behavior.Related Issues
This pull request has the following related issues:
nanmodules forstats/strided#13955Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
I consulted an AI assistant to verify stdlib C header guard conventions, edge cases, and contribution workflows, but the code implementation, stride logic, and test verifications were reviewed and authored manually by myself.
@stdlib-js/reviewers