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.
| 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.
There was a problem hiding this comment.
@0PrashantYadav0 Good point on reusing @stdlib/blas/ext/base/snansumkbn.
Delegating to snansumkbn keeps the codebase DRY, but the main trade-off seems to be single-pass vs. two-pass traversal:
Current inline logic: Does everything in a single pass over x (accumulates Kahan–Babuška sum and counts n simultaneously).
Delegating to snansumkbn: Requires two passes over x (one loop to count non-NaN elements, and a second pass inside snansumkbn to sum).
For large strided arrays, the single-pass inline implementation might be noticeably faster due to better cache locality.
Should we benchmark both approaches to see if the two-pass overhead is significant, or do you prefer prioritizing DRYness here?
There was a problem hiding this comment.
Normally I prefers reusability of code as this is very important for large codebases like us. But again the logic itself is not wrong. For now you should go ahead and do some benchmark for both approaches and based on those result we will discuss this in next office hours. Also you should create a thread in zulip and share all the finding there.
There was a problem hiding this comment.
@0PrashantYadav0 Makes total sense! I've opened the topic Benchmark: single-pass vs two-pass in snanmean* under #dev on Zulip and tagged everyone.
I'm running the benchmarks locally for both single-pass and two-pass variations across different array sizes and will post the results table over in that Zulip thread shortly!
| "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.
Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
stats/strided/snanmeankbn
|
@0PrashantYadav0 Hey Prashant! Whenever you get a chance, could you take a look at the latest updates? Benchmarks: Updated all benchmark files to conform with standard stdlib base generator patterns and verified that local tests and linters pass cleanly. filledarrayBy / rand() question: Left a quick comment on the benchmark thread regarding the filledarrayBy and rand() usage. ndarray.js trade-off: Left my thoughts on the ndarray.js thread comparing the single-pass inline loop (better cache locality) vs. delegating to snansumkbn (two-pass, but DRY). Let me know your thoughts whenever you're free! |
|
@AryanSharma48 Remove the |
Signed-off-by: Aryan Sharma <aryansharma24106@gmail.com>
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