refactor: update `blas/ext/base/sdssum` to follow current project conventions #1727

kunal-511 · 2024-03-06T05:21:13Z

Resolves #1525

Description

What is the purpose of this pull request?

This pull request updates @stdlib/blas/ext/base/sdssum to follow current project conventions.

Related Issues

Does this pull request have any related issues?

This pull request:

resolves [RFC]: refactor blas/ext/base/sdssum to follow current project conventions #1525

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

No.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

Read, understood, and followed the contributing guidelines.

@stdlib-js/reviewers

…t project convention

kunal-511 · 2024-03-06T09:09:28Z

@Planeshifter Please review my PR?

kgryte · 2024-03-06T09:15:17Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.js

-	for ( i = 0; i < x.length; i++ ) {
-		x[ i ] = ( randu()*10.0 ) - 20.0;
-	}
+	var x = filledarrayBy( len, 'float64', rand );


This is incorrect. The dtype should be float32 here and everywhere else in this PR.

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.js

kgryte · 2024-03-06T09:15:51Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.native.js

@@ -36,7 +37,7 @@ var sdssum = tryRequire( resolve( __dirname, './../lib/ndarray.native.js' ) );
 var opts = {
 	'skip': ( sdssum instanceof Error )
 };
-
+var rand = uniform( -100.0, 100.0 );


Suggested change

var rand = uniform( -100.0, 100.0 );

var rand = uniform( -100.0, 100.0 );

kgryte · 2024-03-06T09:16:22Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/examples/index.js

+var y = sdssum( x.length, x, 1 );
+console.log( y );


Suggested change

var y = sdssum( x.length, x, 1 );

console.log( y );

var v = sdssum( x.length, x, 1 );

console.log( v );

No need to change the variable name here.

kgryte · 2024-03-06T09:18:35Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/lib/ndarray.native.js

+	offset = minViewBufferIndex( N, stride, offset );
 	if ( stride < 0 ) {
 		offset += (N-1) * stride;
 	}
-	view = new Float32Array( x.buffer, x.byteOffset+(x.BYTES_PER_ELEMENT*offset), x.length-offset ); // eslint-disable-line max-len
+	view = offsetView( x, offset );


This is the relevant file you want to emulate: https://github.com/stdlib-js/stdlib/blob/58c9a5053430b55c974e9ecccfc45c51c3fefc9e/lib/node_modules/%40stdlib/blas/base/sasum/lib/ndarray.native.js

kgryte · 2024-03-06T09:18:52Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json

@@ -1,5 +1,9 @@
 {
-	"options": {},
+	"options": {


Please convert this file to 2-space indentation.

kgryte · 2024-03-06T09:19:08Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json

+				"@stdlib/napi/export",
+				"@stdlib/napi/argv",
+				"@stdlib/napi/argv-int64",
+				"@stdlib/napi/argv-strided-float64array"


This is incorrect. The input array should be a float32 array.

kgryte · 2024-03-06T09:19:30Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json

 			]
 		}
 	]
-}
+}


Trailing newline. Please ensure you have installed EditorConfig, as mentioned in the development guide.

kgryte · 2024-03-06T09:20:14Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/src/addon.c

+    STDLIB_NAPI_ARGV(env, info, argv, argc, 3);
+    STDLIB_NAPI_ARGV_INT64(env, N, argv, 0);
+    STDLIB_NAPI_ARGV_INT64(env, strideX, argv, 2);
+    STDLIB_NAPI_ARGV_STRIDED_FLOAT64ARRAY(env, X, N, strideX, argv, 1);
+
+    napi_value v;
+    napi_status status = napi_create_double(env, c_sdssum(N, (double *)X, strideX), &v);
+    assert(status == napi_ok);
+
+    return v;


This should be TAB indentation. As mentioned above, input array should be float32, not float64. L43 should be (float *)X.

kgryte · 2024-03-06T09:20:28Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/src/addon.c

+ * @param info   callback data
+ * @return       Node-API value
+ */
+static napi_value addon(napi_env env, napi_callback_info info)


Please adhere to project spacing conventions.

kgryte · 2024-03-06T09:20:36Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/src/addon.c

+    return v;
+}
+
+STDLIB_NAPI_MODULE_EXPORT_FCN(addon)


Trailing newline.

kgryte · 2024-03-06T09:21:45Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/test/test.ndarray.native.js

@@ -45,7 +45,7 @@ tape( 'main export is a function', opts, function test( t ) {
 });

 tape( 'the function has an arity of 4', opts, function test( t ) {
-	t.strictEqual( sdssum.length, 4, 'has expected arity' );
+	t.strictEqual( sdssum.length, 4, 'returns expected value' );


The changes in this file are incomplete. You should refactor to avoid the use of floor. See reference PRs linked to in the OP.

kgryte · 2024-03-06T09:22:00Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/test/test.sdssum.js

@@ -21,7 +21,7 @@
 // MODULES //

 var tape = require( 'tape' );
-var floor = require( '@stdlib/math/base/special/floor' );
+


Suggested change

kgryte · 2024-03-06T09:22:11Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/test/test.sdssum.native.js

@@ -22,7 +22,7 @@

 var resolve = require( 'path' ).resolve;
 var tape = require( 'tape' );
-var floor = require( '@stdlib/math/base/special/floor' );
+


Suggested change

kgryte · 2024-03-06T09:22:47Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/test/test.ndarray.js

@@ -36,7 +36,7 @@ tape( 'main export is a function', function test( t ) {
 });

 tape( 'the function has an arity of 4', function test( t ) {
-	t.strictEqual( sdssum.length, 4, 'has expected arity' );
+	t.strictEqual( sdssum.length, 4, 'returns expected value' );


Refactoring is incomplete. Remove the use of floor.

kgryte

Left various comments. Please pay particular attention to similar PRs and commits and match changes and conventions in those changesets as closely as possible.

kgryte · 2024-03-06T09:25:57Z

Please also check the box in the OP indicating that you have read and understood the contribution guidelines.

kunal-511 · 2024-03-06T09:27:41Z

@kgryte Thanks for your review, I will implement these ass soon as possible

kunal-511 · 2024-03-07T02:29:20Z

@kgryte
The requested changes have been addressed. Please take a look

kgryte · 2024-03-07T03:04:22Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json

@@ -44,8 +44,8 @@
 			"dependencies": [
 				"@stdlib/napi/export",
 				"@stdlib/napi/argv",
-				"@stdlib/napi/argv-int64",
-				"@stdlib/napi/argv-strided-float64array"
+				"@stdlib/napi/argv-int32",


This is incorrect. Should be int64.

kgryte · 2024-03-07T03:04:51Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/src/addon.c

@@ -19,8 +19,8 @@
 #include "stdlib/blas/base/sdssum.h"
 #include "stdlib/napi/export.h"
 #include "stdlib/napi/argv.h"
-#include "stdlib/napi/argv_int64.h"
-#include "stdlib/napi/argv_strided_float64array.h"
+#include "stdlib/napi/argv_int32.h"


This should be int64.

kgryte · 2024-03-07T03:05:05Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/test/test.ndarray.js

@@ -21,7 +21,7 @@
 // MODULES //

 var tape = require( 'tape' );
-var floor = require( '@stdlib/math/base/special/floor' );
+//var floor = require( '@stdlib/math/base/special/floor' );


Don't comment out. Remove.

kgryte · 2024-03-07T03:06:20Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.native.js

 var isnan = require( '@stdlib/math/base/assert/is-nan' );
 var pow = require( '@stdlib/math/base/special/pow' );
-var Float32Array = require( '@stdlib/array/float32' );
+


Suggested change

kgryte · 2024-03-07T03:06:31Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.native.js

-	for ( i = 0; i < x.length; i++ ) {
-		x[ i ] = ( randu()*10.0 ) - 20.0;
-	}
+	var x = filledarrayBy( len, 'float64', rand );


kgryte · 2024-03-07T03:06:46Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.js

 var isnan = require( '@stdlib/math/base/assert/is-nan' );
 var pow = require( '@stdlib/math/base/special/pow' );
-var Float32Array = require( '@stdlib/array/float32' );
+


Suggested change

kgryte · 2024-03-07T03:06:52Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.js

 var pkg = require( './../package.json' ).name;
 var sdssum = require( './../lib/ndarray.js' );


+


Suggested change

Please ensure you have your local dev environment setup, including running make init. Your PR contains various lint errors which project automated tooling can catch, but only if you have properly set things up locally.

kgryte · 2024-03-07T03:07:55Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.js

-	for ( i = 0; i < x.length; i++ ) {
-		x[ i ] = ( randu()*10.0 ) - 20.0;
-	}
+	var x = filledarrayBy( len, 'float64', rand );


kgryte · 2024-03-07T03:08:06Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.native.js

 var isnan = require( '@stdlib/math/base/assert/is-nan' );
 var pow = require( '@stdlib/math/base/special/pow' );
-var Float32Array = require( '@stdlib/array/float32' );
+


Suggested change

kgryte · 2024-03-07T03:08:16Z

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.ndarray.native.js

-	for ( i = 0; i < x.length; i++ ) {
-		x[ i ] = ( randu()*10.0 ) - 20.0;
-	}
+	var x = filledarrayBy( len, 'float64', rand );


lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json

kgryte

This PR currently has a number of issues. Please consult other packages in blas/ext/base and try to adhere as closely as possible. For example, see https://github.com/stdlib-js/stdlib/tree/develop/lib/node_modules/%40stdlib/blas/ext/base/ssumors. But don't just copy-paste. Consider what is appropriate to emulate and where to deviate based on the needs of sdssum. I suggested consulting sasum, but only for how ndarray views are handled.

kunal-511 · 2024-03-07T12:09:32Z

@kgryte @Planeshifter
The requested changes have been addressed. Please take a look

kunal-511 · 2024-03-10T07:26:15Z

@kgryte @Planeshifter Please see this

Planeshifter · 2024-04-20T23:52:15Z

Closing as this refactoring was meanwhile merged per #2080. Thanks either way for your contribution even though it didn't make it in this time!

I have make the necessary changes to refactor sdssum to follow curren…

de359f4

…t project convention

kgryte reviewed Mar 6, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/sdssum/benchmark/benchmark.js Outdated Show resolved Hide resolved

kgryte reviewed Mar 6, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/sdssum/src/addon.c Outdated

return v;

}

STDLIB_NAPI_MODULE_EXPORT_FCN(addon)

Copy link

Member

kgryte Mar 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing newline.

kgryte reviewed Mar 6, 2024

View reviewed changes

kgryte requested changes Mar 6, 2024

View reviewed changes

kgryte added Native Addons Issue involves or relates to Node.js native add-ons. BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). C Issue involves or relates to C. Needs Changes Pull request which needs changes before being merged. labels Mar 6, 2024

kgryte changed the title ~~I have make the necessary changes to refactor sdssum to follow curren…~~ refactor: update blas/ext/base/sdssum to follow current project conventions Mar 6, 2024

Fixed the changes suggested by reviewr

2c3c76a

kunal-511 requested a review from kgryte March 6, 2024 15:53

kgryte mentioned this pull request Mar 6, 2024

[RFC]: refactor blas/ext/base/scusum to follow current project conventions #1516

Closed

3 tasks

Fixed the spacing changes as suggested

fea5262

kgryte reviewed Mar 7, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json Outdated Show resolved Hide resolved

kgryte reviewed Mar 7, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json Outdated Show resolved Hide resolved

kgryte reviewed Mar 7, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/sdssum/manifest.json Outdated Show resolved Hide resolved

kgryte requested changes Mar 7, 2024

View reviewed changes

Fixed the changes as suggested

37c439c

kunal-511 requested a review from kgryte March 7, 2024 04:35

Planeshifter closed this Apr 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update `blas/ext/base/sdssum` to follow current project conventions #1727

refactor: update `blas/ext/base/sdssum` to follow current project conventions #1727

kunal-511 commented Mar 6, 2024 •

edited

Loading

kunal-511 commented Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte Mar 6, 2024

kgryte left a comment

kgryte commented Mar 6, 2024

kunal-511 commented Mar 6, 2024

kunal-511 commented Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte Mar 7, 2024

kgryte left a comment

kunal-511 commented Mar 7, 2024 •

edited

Loading

kunal-511 commented Mar 10, 2024

Planeshifter commented Apr 20, 2024

	var rand = uniform( -100.0, 100.0 );
	var rand = uniform( -100.0, 100.0 );

		var pkg = require( './../package.json' ).name;
		var sdssum = require( './../lib/ndarray.js' );

refactor: update blas/ext/base/sdssum to follow current project conventions #1727

refactor: update blas/ext/base/sdssum to follow current project conventions #1727

Conversation

kunal-511 commented Mar 6, 2024 • edited Loading

Description

Related Issues

Questions

Other

Checklist

kunal-511 commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte left a comment

Choose a reason for hiding this comment

kgryte commented Mar 6, 2024

kunal-511 commented Mar 6, 2024

kunal-511 commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgryte left a comment

Choose a reason for hiding this comment

kunal-511 commented Mar 7, 2024 • edited Loading

kunal-511 commented Mar 10, 2024

Planeshifter commented Apr 20, 2024

refactor: update `blas/ext/base/sdssum` to follow current project conventions #1727

refactor: update `blas/ext/base/sdssum` to follow current project conventions #1727

kunal-511 commented Mar 6, 2024 •

edited

Loading

kunal-511 commented Mar 7, 2024 •

edited

Loading