Skip to content

refactor update blas/ext/base/dsortsh to follow current project conventions #2007

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Apr 28, 2024
Merged

refactor update blas/ext/base/dsortsh to follow current project conventions #2007

merged 10 commits into from
Apr 28, 2024

Conversation

liberocks
Copy link
Contributor

Resolves #1500.

Description

What is the purpose of this pull request?

This pull request refactors the existing implementation to follow the current project convention for blas/ext/base/dsortsh

Related Issues

Does this pull request have any related issues?

This pull request:

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.

@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Mar 23, 2024
@Planeshifter Planeshifter added 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 23, 2024
Pranavchiku
Pranavchiku reviewed Mar 25, 2024
View reviewed changes
...odules/@stdlib/blas/ext/base/dsortsh/benchmark/benchmark.mostly_sorted_few_uniques.native.js Outdated
@@ -38,6 +38,7 @@ var dsortsh = tryRequire( resolve( __dirname, './../lib/dsortsh.native.js' ) );
var opts = {
'skip': ( dsortsh instanceof Error )
};
var rand = uniform( -100.0, 100.0 );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var rand = uniform( -100.0, 100.0 );
var rand = uniform( 1.0, 10.0 );

Pranavchiku
Pranavchiku reviewed Mar 25, 2024
View reviewed changes
Pranavchiku
Pranavchiku reviewed Mar 25, 2024
View reviewed changes
Pranavchiku
Pranavchiku reviewed Mar 25, 2024
View reviewed changes
Pranavchiku
Pranavchiku suggested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@Pranavchiku Pranavchiku left a comment

Choose a reason for hiding this comment

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

Left comments above, this PR is missing updates in readme, docs, etc. Please refer a similar PR and update accordingly. Thanks for your work!

@Pranavchiku 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
@liberocks
Copy link
Contributor Author

Hi @Pranavchiku I have made changes as suggested. However, it turns out we still need this code,

iter = pow( iter, 3.0/4.0 ) | 0;

I tried removing it and the benchmark goes forever. It causes timeout on the CI pipeline. Thus I put it back.

Anyway, thank you for reviewing my PR!

@liberocks liberocks mentioned this pull request Apr 2, 2024
Closed
6 tasks
@Planeshifter Planeshifter self-requested a review April 28, 2024 01:04
Planeshifter
Planeshifter approved these changes Apr 28, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment

Choose a reason for hiding this comment

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

Did some minor clean-up and will merge once CI passes. Thanks for your PR!

@Planeshifter Planeshifter merged commit 6bcf620 into stdlib-js:develop Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pranavchiku Pranavchiku Pranavchiku requested changes

@Planeshifter Planeshifter Planeshifter approved these changes

Assignees
No one assigned
Labels
BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). C Issue involves or relates to C. Native Addons Issue involves or relates to Node.js native add-ons. Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC]: refactor blas/ext/base/dsortsh to follow current project conventions
4 participants
@liberocks @Planeshifter @Pranavchiku @stdlib-bot