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/dge-trans #2734

Merged
merged 28 commits into from
Aug 12, 2024
Merged

Conversation

Pranavchiku
Copy link
Member

@Pranavchiku Pranavchiku commented Aug 3, 2024

Towards #2464.

Description

What is the purpose of this pull request?

This pull request exposes utility function dge_trans as lapack/base/dge-trans.

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 3, 2024
@kgryte
Copy link
Member

kgryte commented Aug 3, 2024

@Pranavchiku Is there a reason to name the package as transpose rather than dge-trans? We'll need separate packages for single-precision and complex dtypes.

@Pranavchiku
Copy link
Member Author

I see, I initially used snake case and it threw me lint error. Also, I am unable to understand sense of dge-*, can we not have dtrans?

@Pranavchiku Pranavchiku marked this pull request as ready for review August 3, 2024 05:49
@Pranavchiku
Copy link
Member Author

Pranavchiku commented Aug 3, 2024

Is it double precision general matrix transpose? If yes, then it makes sense.

@Pranavchiku Pranavchiku changed the title feat: add lapack/base/transpose feat: add lapack/base/dge-trans Aug 3, 2024
@kgryte
Copy link
Member

kgryte commented Aug 3, 2024

Is it double precision general matrix transpose? If yes, then it makes sense.

Yes, I believe so. Not sure why they didn't do dgetrans, but 🤷‍♂️ .

@Pranavchiku Pranavchiku changed the title feat: add lapack/base/dge-trans feat: add lapack/base/dgetrans Aug 3, 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.

Left some initial comments.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Aug 3, 2024
@Pranavchiku Pranavchiku added Needs Review A pull request which needs code review. and removed Needs Changes Pull request which needs changes before being merged. labels Aug 4, 2024
@kgryte kgryte changed the title feat: add lapack/base/dgetrans feat: add lapack/base/dge-trans Aug 12, 2024
@kgryte kgryte removed the Needs Review A pull request which needs code review. label Aug 12, 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 for working on this, @Pranavchiku.

Note that the tests were rather spartan. I would argue it is better to create tests which build on one another, rather than creating a single test which attempts to do all the things. By isolating one behavioral aspect at a time, we can better ensure that an implementation behaves as expected. For combining all the things into one, it can be harder to disentangle which aspect is causing a test to fail.

@kgryte
Copy link
Member

kgryte commented Aug 12, 2024

In principle, we could implement matrix transposition using loop tiling. Loop tiling would only benefit when layout access patterns for A and out differ. E.g., if both A and out are the same (e.g., row-major), because we are taking the transpose, the access pattern for A will be column-major. In this instance, loop tiling can be beneficial. Otherwise, a naive nested loop is cache optimal. Regardless, I'll leave implementing loop tiling to a follow-up PR.

@kgryte kgryte merged commit cd5ad1c into stdlib-js:develop Aug 12, 2024
11 checks passed
gunjjoshi pushed a commit to gunjjoshi/stdlib that referenced this pull request Aug 21, 2024
PR-URL: stdlib-js#2734
Ref: stdlib-js#2464
Co-authored-by: Athan Reines <[email protected]>
Reviewed-by: Athan Reines <[email protected]>
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).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants