Skip to content
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

feat: add blas/base/srot #1844

Merged
merged 16 commits into from
Apr 20, 2024
Merged

feat: add blas/base/srot #1844

merged 16 commits into from
Apr 20, 2024

Conversation

aman-095
Copy link
Member

Resolves #277.

Description

This RFC proposes to add a routine to apply a plane rotation as defined in BLAS Level 1 routines. Specifically adding @stdlib/blas/base/srot is proposed.

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

@Planeshifter Planeshifter added Needs Review A pull request which needs code review. 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. labels Mar 12, 2024
@aman-095
Copy link
Member Author

aman-095 commented Mar 13, 2024

CC @kgryte @Pranavchiku

Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

As this PR likely requires all of the same changes as requested in #1823, I'll mark this PR accordingly.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Mar 14, 2024
@kgryte kgryte removed Needs Changes Pull request which needs changes before being merged. Needs Review A pull request which needs code review. labels Apr 20, 2024
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @aman-095!

One comment: test tolerances should be consistent across comparable API tests. E.g., if we use 5.0 * EPS for srot.js, we should use 5.0 * EPS for an equivalent test in ndarray.js. Similarly, if a native binding requires 5.0 * EPS to pass (e.g., due to Fortran divergence from C), that same tolerance should be used for the JavaScript implementation. Tolerances serve two purposes:

  1. To sanity check that we have reasonable results relative to a reference implementation/expected value. If we observe significant divergence (e.g., 100.0 * EPS) that is a code smell that something is going wrong.
  2. To project against future regressions.

In this case, our tests act as our stated contract with end users. If we can only "guarantee" 5.0 * EPS agreement with a reference implementation due to Fortran shenanigans, that should be the case across all implementations.

@kgryte kgryte merged commit d1cbb09 into stdlib-js:develop Apr 20, 2024
8 checks passed
@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. Native Addons Issue involves or relates to Node.js native add-ons.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: add BLAS Level 1 srot routine
3 participants