-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add blas/ext/base/ndarray/dapx
#9220
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?
feat: add blas/ext/base/ndarray/dapx
#9220
Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
| var dapx = require( '@stdlib/blas/ext/base/ndarray/dapx' ); | ||
| ``` | ||
|
|
||
| #### dapx( x, alpha ) |
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 isn't correct. You need to study https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/blas/ext/base/ndarray/dcusum. alpha should be a zero-dimensional ndarray and there should be only a single argument arrays containing two arrays: the input ndarray and the zero-dimensional ndarray corresponding to alpha.
This comment applies to your other PRs for *apx.
lib/node_modules/@stdlib/blas/ext/base/ndarray/dapx/benchmark/benchmark.js
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,27 @@ | |||
| {{alias}}( x, alpha ) | |||
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 empty line. Study other packages.
|
|
||
| // The function returns an ndarray... | ||
| { | ||
| const x: any = null; |
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 very clearly not what we want to do.
|
|
||
| // The compiler throws an error if the function is provided a second argument which is not a number... | ||
| { | ||
| const x: any = null; |
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.
| /** | ||
| * Adds a scalar constant to each element in a double-precision floating-point ndarray. | ||
| * | ||
| * @param {float64ndarray} x - input ndarray |
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 isn't a valid type for JSDoc. Study other packages.
| var buf; | ||
| var sx; | ||
| var ox; | ||
| var N; | ||
|
|
||
| N = numelDimension( x, 0 ); | ||
| if ( N <= 0 ) { | ||
| return x; | ||
| } | ||
| buf = getData( x ); | ||
| sx = getStride( x, 0 ); | ||
| ox = getOffset( x ); | ||
|
|
||
| strided( N, alpha, buf, sx, ox ); | ||
| return x; |
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.
Study other packages.
kgryte
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.
Left initial comments. This PR (and related PRs) need significant clean-up before they can move forward.
|
Thanks for the detailed guidance @kgryte. It really clarified the architectural pattern for me. I have refactored the implementation to align with dcusum and other packages. The function now accepts a single arrays argument, and I am using ndarraylike2scalar for correct value extraction. I have also updated the JSDoc types and examples (using the new notation) and will apply these changes across all my related *apx PRs. This one is Ready for review. |
|
|
||
| # dapx | ||
|
|
||
| > Add a scalar constant to each element in a double-precision floating-point ndarray. |
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.
| > Add a scalar constant to each element in a double-precision floating-point ndarray. | |
| > Add a scalar constant to each element in a one-dimensional double-precision floating-point ndarray. |
Applies here and throughout this PR.
| Note that indexing is relative to the first index. To introduce an offset, use [`ndarray`][@stdlib/ndarray/ctor] view creation. | ||
|
|
||
| ```javascript | ||
| var Float64Array = require( '@stdlib/array/float64' ); | ||
| var ndarray = require( '@stdlib/ndarray/ctor' ); | ||
| var scalar2ndarray = require( '@stdlib/ndarray/from-scalar' ); | ||
|
|
||
| // Initial array: | ||
| var xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0, 5.0 ] ); | ||
|
|
||
| // Create an ndarray view: | ||
| var x = new ndarray( 'float64', xbuf, [ 3 ], [ 1 ], 2, 'row-major' ); | ||
|
|
||
| var alpha = scalar2ndarray( 5.0, { | ||
| 'dtype': 'float64' | ||
| }); | ||
|
|
||
| var out = dapx( [ x, alpha ] ); | ||
| // returns <ndarray> | ||
| ``` |
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.
| Note that indexing is relative to the first index. To introduce an offset, use [`ndarray`][@stdlib/ndarray/ctor] view creation. | |
| ```javascript | |
| var Float64Array = require( '@stdlib/array/float64' ); | |
| var ndarray = require( '@stdlib/ndarray/ctor' ); | |
| var scalar2ndarray = require( '@stdlib/ndarray/from-scalar' ); | |
| // Initial array: | |
| var xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0, 5.0 ] ); | |
| // Create an ndarray view: | |
| var x = new ndarray( 'float64', xbuf, [ 3 ], [ 1 ], 2, 'row-major' ); | |
| var alpha = scalar2ndarray( 5.0, { | |
| 'dtype': 'float64' | |
| }); | |
| var out = dapx( [ x, alpha ] ); | |
| // returns <ndarray> | |
| ``` |
| ## Notes | ||
|
|
||
| - The function **mutates** the input ndarray. | ||
|
|
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.
| ## Notes | |
| - The function **mutates** the input ndarray. |
| }); | ||
| var x = new ndarray( 'float64', xbuf, [ 10 ], [ 1 ], 0, 'row-major' ); | ||
|
|
||
| console.log( x.data ); |
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.
Study other packages. Use ndarray2array.
|
|
||
| The function has the following parameters: | ||
|
|
||
| - **arrays**: array-like object containing an input ndarray and a zero-dimensional ndarray containing the scalar constant. |
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.
| - **arrays**: array-like object containing an input ndarray and a zero-dimensional ndarray containing the scalar constant. | |
| - **arrays**: array-like object containing a one-dimensional input ndarray and a zero-dimensional ndarray containing a scalar constant. |
| }); | ||
|
|
||
| var out = dapx( [ x, alpha ] ); | ||
| // returns <ndarray> |
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 <ndarray> | |
| // returns <ndarray>[ 6.0, 7.0, 8.0, 9.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.
Show what the expected values are.
| var dapx = require( './../lib' ); | ||
|
|
||
|
|
||
| // FUNCTIONS // |
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.
| // FUNCTIONS // | |
| // VARIABLES // | |
| var options = { | |
| 'dtype': 'float64' | |
| }; | |
| // FUNCTIONS // |
Prefer parameterization to limit copy-paste mistakes in other packages. This is one reason to first do one package (e.g., dapx) before moving on to other packages (e.g., sapx, gapx), as it reduces the number of changes you need to make. Currently, you are having to replicate changes.
| xbuf = discreteUniform( len, -100, 100, { | ||
| 'dtype': 'float64' | ||
| }); | ||
| x = new ndarray( 'float64', xbuf, [ len ], [ 1 ], 0, 'row-major' ); | ||
| alpha = scalar2ndarray( 5.0, { | ||
| 'dtype': 'float64' | ||
| }); |
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.
| xbuf = discreteUniform( len, -100, 100, { | |
| 'dtype': 'float64' | |
| }); | |
| x = new ndarray( 'float64', xbuf, [ len ], [ 1 ], 0, 'row-major' ); | |
| alpha = scalar2ndarray( 5.0, { | |
| 'dtype': 'float64' | |
| }); | |
| xbuf = discreteUniform( len, -100, 100, options ); | |
| x = new ndarray( options.dtype, xbuf, [ len ], [ 1 ], 0, 'row-major' ); | |
| alpha = scalar2ndarray( 5.0, options ); |
|
|
||
| var bench = require( '@stdlib/bench' ); | ||
| var discreteUniform = require( '@stdlib/random/array/discrete-uniform' ); | ||
| var isnanf = require( '@stdlib/math/base/assert/is-nanf' ); |
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.
It is not clear to me why you are using a single-precision assertion package here.
| @@ -0,0 +1,27 @@ | |||
| {{alias}}( arrays ) | |||
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.
See other packages. This was not the correct change.
| > var x = {{alias:@stdlib/ndarray/ctor}}( 'float64', xbuf, [ 4 ], [ 1 ], 0, 'row-major' ); | ||
| > var alpha = {{alias:@stdlib/ndarray/from-scalar}}( 5.0, { 'dtype': 'float64' } ); | ||
| > {{alias}}( [ x, alpha ] ) | ||
| <ndarray> |
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.
Show expected values.
| * var out = dapx( [ x, alpha ] ); | ||
| * // returns <ndarray> | ||
| */ | ||
| declare function dapx( arrays: ArrayLike<float64ndarray> ): float64ndarray; |
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.
| declare function dapx( arrays: ArrayLike<float64ndarray> ): float64ndarray; | |
| declare function dapx( arrays: [ float64ndarray, float64ndarray ] ): float64ndarray; |
By using ArrayLike, you end up losing type specificity. Here, we can specify that one must provide exactly two ndarrays.
| * }); | ||
| * | ||
| * var out = dapx( [ x, alpha ] ); | ||
| * // returns <ndarray> |
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.
Show expected values.
| * }); | ||
| * | ||
| * var out = dapx( [ x, alpha ] ); | ||
| * // returns <ndarray> |
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.
Show expected values.
| /** | ||
| * Adds a scalar constant to each element in a double-precision floating-point ndarray. | ||
| * | ||
| * @param {ArrayLikeObject<Object>} arrays - array-like object containing an input ndarray and a zero-dimensional ndarray containing the scalar constant |
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.
| * @param {ArrayLikeObject<Object>} arrays - array-like object containing an input ndarray and a zero-dimensional ndarray containing the scalar constant | |
| * @param {ArrayLikeObject<Object>} arrays - array-like object containing a one-dimensional input ndarray and a zero-dimensional ndarray containing the scalar constant |
| // MAIN // | ||
|
|
||
| /** | ||
| * Adds a scalar constant to each element in a double-precision floating-point ndarray. |
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.
| * Adds a scalar constant to each element in a double-precision floating-point ndarray. | |
| * Adds a scalar constant to each element in a one-dimensional double-precision floating-point ndarray. |
| * }); | ||
| * | ||
| * var out = dapx( [ x, alpha ] ); | ||
| * // returns <ndarray> |
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.
Show expected values.
|
|
||
| if ( numelDimension( x, 0 ) <= 0 ) { | ||
| return x; | ||
| } |
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 ( numelDimension( x, 0 ) <= 0 ) { | |
| return x; | |
| } |
| { | ||
| "name": "@stdlib/blas/ext/base/ndarray/dapx", | ||
| "version": "0.0.0", | ||
| "description": "Add a scalar constant to each element in a double-precision floating-point ndarray.", |
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.
| "description": "Add a scalar constant to each element in a double-precision floating-point ndarray.", | |
| "description": "Add a scalar constant to each element in a one-dimensional double-precision floating-point ndarray.", |
| "constant", | ||
| "plus", | ||
| "increment", | ||
| "augment", |
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.
| "augment", |
| "array", | ||
| "multidimensional", | ||
| "add", | ||
| "constant", |
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.
| "constant", |
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'the function adds a scalar constant to each element in a double-precision floating-point ndarray', 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 adds a scalar constant to each element in a double-precision floating-point ndarray', function test( t ) { | |
| tape( 'the function adds a scalar constant to each element in an ndarray', 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.
Minimize future copy-paste errors.
|
|
||
| dapx( [ x, alpha ] ); | ||
|
|
||
| t.deepEqual( x.data, expected, '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.
Use ndarray2array or getData (see @stdlib/ndarray/data-buffer). Meaning, prefer functional accessors in order to minimize future migration burden should we ever rename properties, etc.
| t.end(); | ||
| }); | ||
|
|
||
| tape( 'the function supports ndarrays with an offset', 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 supports ndarrays with an offset', function test( t ) { | |
| tape( 'the function supports ndarrays with non-zero offsets', function test( t ) { |
| tape( 'the function supports adding zero', function test( t ) { | ||
| var expected; | ||
| var alpha; | ||
| var xbuf; | ||
| var x; | ||
|
|
||
| xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | ||
| x = new ndarray( 'float64', xbuf, [ 4 ], [ 1 ], 0, 'row-major' ); | ||
|
|
||
| expected = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | ||
|
|
||
| alpha = scalar2ndarray( 0.0, { | ||
| 'dtype': 'float64' | ||
| }); | ||
|
|
||
| dapx( [ x, alpha ] ); | ||
|
|
||
| t.deepEqual( x.data, expected, 'returns expected value' ); | ||
| t.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.
| tape( 'the function supports adding zero', function test( t ) { | |
| var expected; | |
| var alpha; | |
| var xbuf; | |
| var x; | |
| xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | |
| x = new ndarray( 'float64', xbuf, [ 4 ], [ 1 ], 0, 'row-major' ); | |
| expected = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | |
| alpha = scalar2ndarray( 0.0, { | |
| 'dtype': 'float64' | |
| }); | |
| dapx( [ x, alpha ] ); | |
| t.deepEqual( x.data, expected, 'returns expected value' ); | |
| t.end(); | |
| }); |
This test is not particularly informative.
| tape( 'the function supports negative constants', function test( t ) { | ||
| var expected; | ||
| var alpha; | ||
| var xbuf; | ||
| var x; | ||
|
|
||
| xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | ||
| x = new ndarray( 'float64', xbuf, [ 4 ], [ 1 ], 0, 'row-major' ); | ||
|
|
||
| expected = new Float64Array( [ -4.0, -3.0, -2.0, -1.0 ] ); | ||
|
|
||
| alpha = scalar2ndarray( -5.0, { | ||
| 'dtype': 'float64' | ||
| }); | ||
|
|
||
| dapx( [ x, alpha ] ); | ||
|
|
||
| t.deepEqual( x.data, expected, 'returns expected value' ); | ||
| t.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.
| tape( 'the function supports negative constants', function test( t ) { | |
| var expected; | |
| var alpha; | |
| var xbuf; | |
| var x; | |
| xbuf = new Float64Array( [ 1.0, 2.0, 3.0, 4.0 ] ); | |
| x = new ndarray( 'float64', xbuf, [ 4 ], [ 1 ], 0, 'row-major' ); | |
| expected = new Float64Array( [ -4.0, -3.0, -2.0, -1.0 ] ); | |
| alpha = scalar2ndarray( -5.0, { | |
| 'dtype': 'float64' | |
| }); | |
| dapx( [ x, alpha ] ); | |
| t.deepEqual( x.data, expected, 'returns expected value' ); | |
| t.end(); | |
| }); |
Neither is this one.
kgryte
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.
Left another round of comments.
|
Actually, one package you should study is |
- Add 'one-dimensional' to all descriptions - Use ndarray2array to show expected values - Remove dimension check from main function - Update TypeScript types to tuple syntax - Use getData instead of .data in tests - Remove uninformative test cases - Add VARIABLES section in benchmark - Use options parameterization - Change isnanf to isnan for double precision - Remove Notes section from README - Update all documentation with expected outputs
83f8437 to
9943604
Compare
|
Thanks for the extensive review @kgryte. I have refactored this package
I have applied these changes ONLY to this PR first. Once you confirm this implementation meets the standard, I will immediately propagate the exact same pattern to the other pending PRs (sapx, gapx, etc.) to ensure consistency. |
Resolves None.
Description
This pull request:
blas/ext/base/ndarray/dapxRelated 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
@stdlib-js/reviewers