refactor: update `blas/ext/base/dsnansum` to follow current project conventions #2001

Shubh942 · 2024-03-22T17:39:01Z

Resolves #1492.

Description

update blas/ext/base/dsnansum to follow current project conventions

What is the purpose of this pull request?

This pull request:

progresses #1152
Fixes #1492

Related Issues

Does this pull request have any related issues?

This pull request:

resolves #
fixes #

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

…entions

Shubh942 · 2024-03-22T17:40:27Z

@Planeshifter @Pranavchiku @kgryte can you please review it

Pranavchiku

You have to fix addon.c file, refer similar PRs. Left few comments, once addressed this will be good to merge.

lib/node_modules/@stdlib/blas/ext/base/dsnansum/README.md

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

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

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

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

Shubh942 · 2024-03-25T23:52:05Z

I will change the code as specified. Thanks for your response

Shubh942 · 2024-03-26T10:38:21Z

@Pranavchiku I have made the suggested changes.

Pranavchiku

You will have to update indentation in manifest.json and update addon.c

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

…ventions

Shubh942 · 2024-03-26T12:21:57Z

@Pranavchiku I have updated the indentation in manifest.json and update addon.c.

Shubh942 · 2024-03-27T07:32:51Z

@Planeshifter can you please review my PR

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

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

Signed-off-by: Pranav <85227306+Pranavchiku@users.noreply.github.com>

Pranavchiku

This looks good to me. Thanks @Shubh942 !

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

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

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

kgryte · 2024-03-29T04:00:59Z

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

+      "dependencies": [
+        "@stdlib/napi/export",
+        "@stdlib/napi/argv",
+        "@stdlib/napi/argv-float",


Suggested change

"@stdlib/napi/argv-float",

This isn't needed.

kgryte · 2024-03-29T04:01:45Z

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

+#include "stdlib/blas/ext/base/dsnansum.h"
+#include "stdlib/napi/export.h"
+#include "stdlib/napi/argv.h"
+#include "stdlib/napi/argv_float.h"


Suggested change

#include "stdlib/napi/argv_float.h"

This isn't used.

kgryte · 2024-03-29T04:03:30Z

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

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

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


Suggested change

t.strictEqual( dsnansum.length, 4, 'return expected value' );

t.strictEqual( dsnansum.length, 4, 'returns expected value' );

kgryte · 2024-03-29T04:03:46Z

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

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

 tape( 'the function has an arity of 3', function test( t ) {
-	t.strictEqual( dsnansum.length, 3, 'has expected arity' );
+	t.strictEqual( dsnansum.length, 3, 'return expected value' );


Suggested change

t.strictEqual( dsnansum.length, 3, 'return expected value' );

t.strictEqual( dsnansum.length, 3, 'returns expected value' );

kgryte · 2024-03-29T04:04:13Z

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

@@ -44,7 +43,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( dsnansum.length, 4, 'has expected arity' );
+	t.strictEqual( dsnansum.length, 4, 'return expected value' );


Suggested change

t.strictEqual( dsnansum.length, 4, 'return expected value' );

t.strictEqual( dsnansum.length, 4, 'returns expected value' );

kgryte · 2024-03-29T04:04:52Z

lib/node_modules/@stdlib/blas/ext/base/dsnansum/README.md

-        x[ i ] = round( randu()*100.0 );
-    }
-}
+var x = filledarrayBy( 10, 'float32', discreteUniform( 0, 100 ) );


The changes to this example are not equivalent to the original.

kgryte · 2024-03-29T04:05:13Z

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

-			x[ i ] = ( randu()*10.0 ) - 20.0;
-		}
-	}
+	var x = filledarrayBy( len, 'float32', rand );


The changes to this (and the other benchmarks) are not equivalent to the original.

kgryte · 2024-03-29T04:07:24Z

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

-			],
-			"libpath": [],
-			"dependencies": [
-				"@stdlib/blas/ext/base/dsnansumpw"


This dependency has been omitted from the dependencies in each of the configurations. This should be re-added to each of them.

kgryte

This PR needs changes, as directed in the comments, before it can be merged. Benchmark test data generation should remain similar to the original. Namely, NaNs should be present; otherwise, compiler optimizations can result in conditionals being removed, resulting in better measured performance than "real-world" usage.

Shubh942 · 2024-03-29T11:09:20Z

@kgryte @Planeshifter I made the required changes. Can you please review it.

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

Shubh942 · 2024-03-30T09:51:16Z

@kgryte @Planeshifter I made the required changes.

kgryte · 2024-04-08T02:10:30Z

@Shubh942 Looks like this PR is failing CI, including failing tests and lint errors. Until those are resolved, we won't be able to move this PR forward.

refactor update blas/ext/base/dsnansum to follow current project conv…

636ce44

…entions

stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Mar 22, 2024

kgryte changed the title ~~refactor update blas/ext/base/dsnansum to follow current project conventions~~ refactor: update blas/ext/base/dsnansum to follow current project conventions Mar 22, 2024

kgryte added Enhancement Issue or pull request for enhancing existing functionality. Needs Review A pull request which needs code review. Native Addons Issue involves or relates to Node.js native add-ons. C Issue involves or relates to C. labels Mar 22, 2024

Pranavchiku suggested changes Mar 25, 2024

View reviewed changes

Pranavchiku added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Mar 25, 2024

Shubh942 mentioned this pull request Mar 25, 2024

[RFC]: Make code blocks on website documentation interactive stdlib-js/google-summer-of-code#60

Closed

6 tasks

required changes has been done

87fa2e4

Pranavchiku suggested changes Mar 26, 2024

View reviewed changes

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

refactor: update blas/ext/base/dsnansum to follow current project con…

6274b13

…ventions

Pranavchiku reviewed Mar 27, 2024

View reviewed changes

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

Pranavchiku reviewed Mar 27, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/dsnansum/src/addon.c Outdated Show resolved Hide resolved

chore: apply suggestions from code review

742f2a9

Signed-off-by: Pranav <85227306+Pranavchiku@users.noreply.github.com>

Pranavchiku approved these changes Mar 27, 2024

View reviewed changes

Pranavchiku added Ready To Merge A pull request which is ready to be merged. and removed Needs Changes Pull request which needs changes before being merged. labels Mar 27, 2024

Planeshifter added 5 commits March 28, 2024 17:38

docs: add empty line

96ba081

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

test: remove N declaration

8779f57

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

test: remove N declaration

ebdd985

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

test: remove N declaration

83fbeef

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

test: remove N declaration

6f2cdd5

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

Planeshifter reviewed Mar 28, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/dsnansum/src/addon.c Outdated Show resolved Hide resolved

Planeshifter reviewed Mar 28, 2024

View reviewed changes

lib/node_modules/@stdlib/blas/ext/base/dsnansum/src/addon.c Outdated Show resolved Hide resolved

Planeshifter added 3 commits March 28, 2024 21:29

fix: update addon.c

9651840

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

fix: update addon.c

3728d30

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

fix: remove extra closing brace

9f913c4

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

kgryte reviewed Mar 29, 2024

View reviewed changes

kgryte added Needs Changes Pull request which needs changes before being merged. and removed Ready To Merge A pull request which is ready to be merged. labels Mar 29, 2024

kgryte reviewed Mar 29, 2024

View reviewed changes

kgryte requested changes Mar 29, 2024

View reviewed changes

refactor

5c1abbd

Planeshifter and others added 2 commits March 29, 2024 09:38

style: fix indentation

3d554da

Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>

corrected the indentation

57cf939

Planeshifter self-requested a review April 14, 2024 20:35

Planeshifter added 5 commits April 14, 2024 16:44

chore: minor clean-up to get CI to pass

8daf6d8

test: fix passed argument

b0ecb18

chore: fix array type

a08872d

fix: add dependency to manifest.json configs

cbe8f3b

fix: add missing field

6d8e649

Planeshifter approved these changes Apr 14, 2024

View reviewed changes

Planeshifter merged commit d2ec36e into stdlib-js:develop Apr 14, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update `blas/ext/base/dsnansum` to follow current project conventions #2001

refactor: update `blas/ext/base/dsnansum` to follow current project conventions #2001

Shubh942 commented Mar 22, 2024

Shubh942 commented Mar 22, 2024

Pranavchiku left a comment

Shubh942 commented Mar 25, 2024

Shubh942 commented Mar 26, 2024

Pranavchiku left a comment

Shubh942 commented Mar 26, 2024

Shubh942 commented Mar 27, 2024

Pranavchiku left a comment

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte Mar 29, 2024

kgryte left a comment •

edited

Loading

Shubh942 commented Mar 29, 2024

Shubh942 commented Mar 30, 2024

kgryte commented Apr 8, 2024

	t.strictEqual( dsnansum.length, 4, 'return expected value' );
	t.strictEqual( dsnansum.length, 4, 'returns expected value' );

	t.strictEqual( dsnansum.length, 3, 'return expected value' );
	t.strictEqual( dsnansum.length, 3, 'returns expected value' );

refactor: update blas/ext/base/dsnansum to follow current project conventions #2001

refactor: update blas/ext/base/dsnansum to follow current project conventions #2001

Conversation

Shubh942 commented Mar 22, 2024

Description

Related Issues

Questions

Other

Checklist

Shubh942 commented Mar 22, 2024

Pranavchiku left a comment

Choose a reason for hiding this comment

Shubh942 commented Mar 25, 2024

Shubh942 commented Mar 26, 2024

Pranavchiku left a comment

Choose a reason for hiding this comment

Shubh942 commented Mar 26, 2024

Shubh942 commented Mar 27, 2024

Pranavchiku left a 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 • edited Loading

Choose a reason for hiding this comment

Shubh942 commented Mar 29, 2024

Shubh942 commented Mar 30, 2024

kgryte commented Apr 8, 2024

refactor: update `blas/ext/base/dsnansum` to follow current project conventions #2001

refactor: update `blas/ext/base/dsnansum` to follow current project conventions #2001

kgryte left a comment •

edited

Loading