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 lapack/base/dlaev2 #2828

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Pranavchiku
Copy link
Member

Towards #2464.

Description

What is the purpose of this pull request?

This PR adds JS implementation for lapack/base/dlaev2

Related Issues

Does this pull request have any related issues?

NA

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

@Pranavchiku Pranavchiku added Feature Issue or pull request for adding a new feature. Base Issue or pull requests related to "low-level" functionality oriented toward library consumers. JavaScript Issue involves or relates to JavaScript. LAPACK Issue or pull request related to the Linear Algebra Package (LAPACK). labels Aug 24, 2024
@Pranavchiku Pranavchiku marked this pull request as ready for review August 24, 2024 04:30

var ev = new Float64Array( 2 );
var out = new Float64Array( 2 );
out = dlaev2( 2.0, 3.0, 4.0, out, ev );
Copy link
Member

Choose a reason for hiding this comment

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

Do something different; otherwise, what is the point of having this code block?

b.tic();
for ( i = 0; i < b.iterations; i++ ) {
out = dlaev2( A, B, C, out, ev );
A = out[ i%out.length ];
Copy link
Member

Choose a reason for hiding this comment

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

Why are we recycling out values?

b.tic();
for ( i = 0; i < b.iterations; i++ ) {
out = dlaev2( A, B, C, out, 1, 0, ev, 1, 0 );
A = out[ i%out.length ];
Copy link
Member

Choose a reason for hiding this comment

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

Same comment.

- **B**: the (0,1) and the conjugate of (1,0) element of a 2x2 symmetric matrix.
- **C**: the (1,1) element of a 2x2 symmetric matrix.
- **out**: output [`Float64Array`][mdn-float64array] containing the eigenvalues of larger and smaller absolute values respectively.
- **ev**: output [`Float64Array`][mdn-float64array] containing `CS1` and `SN1` which is unit right eigenvector for `RT1` giving the decomposition.
Copy link
Member

@kgryte kgryte Sep 17, 2024

Choose a reason for hiding this comment

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

You're going to need to explain CS1 and SN1 and RT1. This description doesn't tell me a whole lot. Either add prose or show equations.

acmn = A;
}
if ( adf > ab ) {
rt = adf * sqrt( 1.0 + pow( ( ab / adf ), 2 ) );
Copy link
Member

Choose a reason for hiding this comment

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

Use abs2 here and elsewhere.

rt = ab * sqrt( 1.0 + pow( ( adf / ab ), 2 ) );
} else {
// Includes case AB = ADF = 0
rt = ab * sqrt( 2.0 );
Copy link
Member

Choose a reason for hiding this comment

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

Use the constant SQRT2.

out = new Float64Array( 2 );
ev = new Float64Array( 2 );
out = dlaev2( 2.0, 3.0, 4.0, out, ev );
expected = new Float64Array( [ 6.16227766016838, -0.1622776601683793 ] );
Copy link
Member

Choose a reason for hiding this comment

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

Where do the expected values come from?

t.end();
});

tape( 'the function supports complex access pattern to store computed values', function test( t ) {
Copy link
Member

Choose a reason for hiding this comment

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

Improve these tests as discussed on other PRs.

Copy link
Member

Choose a reason for hiding this comment

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

E.g., #2833

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Base Issue or pull requests related to "low-level" functionality oriented toward library consumers. Feature Issue or pull request for adding a new feature. JavaScript Issue involves or relates to JavaScript. LAPACK Issue or pull request related to the Linear Algebra Package (LAPACK). Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants