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/csscal #2169

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

performant23
Copy link
Contributor

@performant23 performant23 commented Apr 15, 2024

If applied, this commit will add the package blas/base/csscal which effectively scales a complex single-precision floating-point vector by a real single-precision floating-point constant.

Description

This pull request:

  • includes the support for C and Fortran and obtains Fortran reference from this page

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

If applied, this commit will add the package `blas/base/csscal` which effectively scales a complex single-precision floating-point vector by a real single-precision floating-point constant.
@stdlib-bot stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Apr 15, 2024
@performant23
Copy link
Contributor Author

performant23 commented Apr 15, 2024

Hello, I have to do some styling changes in the package like spaces before any kind of brackets in the JS implementations and write README.md but since I got some issues while running, just thought I should open up a draft PR. So, I request you to ignore those for the time being.

Some notes:

  • The test data is taken from PR for cscal.
  • For representing the input array, we have used x/X (and x0, x1) for JS and C while using cx since in packages like ccopy that same convention is followed. If we're requiring the use of names cx/CX, please do let me know so that I can make the changes on them before code review (since there are a lot of those instances).
  1. For test.ndarray.native.js, I am getting this error for both Fortran and C:
	return new Float32Array( x.buffer, x.byteOffset+(x.BYTES_PER_ELEMENT*offset), 2*(x.length-offset) ); // eslint-disable-line max-len
	       ^

RangeError: Start offset -24 is outside the bounds of the buffer
    at new Float32Array (<anonymous>)
    at reinterpret (d:\stdlib\lib\node_modules\@stdlib\strided\base\reinterpret-complex64\lib\main.js:47:9)
    at csscal (d:\stdlib\lib\node_modules\@stdlib\blas\base\csscal\lib\ndarray.native.js:63:10)
    at Test.test (d:\stdlib\lib\node_modules\@stdlib\blas\base\csscal\test\test.ndarray.native.js:254:2)
    at Test.bound [as _cb] (d:\stdlib\node_modules\tape\lib\test.js:58:32)
    at Test.run (d:\stdlib\node_modules\tape\lib\test.js:77:10)
    at Test.bound [as run] (d:\stdlib\node_modules\tape\lib\test.js:58:32)
    at Immediate.next (d:\stdlib\node_modules\tape\lib\results.js:70:15)
    at process.processImmediate (node:internal/timers:476:21)

when i keep stride less than 0 like csscal( 4, alpha, x, -1, 0 );. I'm not sure how to approach this since it this means that in reinterpret, a negative offset value is invalid (maybe we could directly return x without any run in ndarray.native.js like:

function csscal( N, alpha, x, strideX, offsetX ) {
	var viewX;
	offsetX = minViewBufferIndex( N, strideX, offsetX );
	if (offsetX < 0) {
		return x;
	}
	viewX = reinterpret( x, offsetX );
	addon( N, new Float32Array([alpha]), viewX, strideX );
	return x;
}
  1. Also, I'm hilariously getting an error related to modules not found while running C implementation which is also found in the PR for cscal. I double-checked the .json files and the filepaths but the error seems persistent.

Edit: For the C lint error, I tried defining alpha_len inside the function scope, directly typecasting to int64_t, and passing alpha_len's address but still gives errors to build the add-ons.

⭐ Since the PR for cscal itself is under review and there was no corresponding z* implementation, I expect this PR to be fairly involved but yeah, it would be really helpful too in terms of understanding how c* and in general, how blas routines using complex numbers are implemented. So, thank you so much in advance!

@kgryte kgryte added Feature Issue or pull request for adding a new feature. Native Addons Issue involves or relates to Node.js native add-ons. Fortran Issue involves or relates to Fortran. C Issue involves or relates to C. labels Apr 20, 2024
@kgryte
Copy link
Member

kgryte commented Jun 11, 2024

@performant23 Sorry for the long turnaround. PRs for cscal and zscal have now been merged. It is likely worth checking out the final implementations as currently in the develop branch. Updating based on those implementations may help resolve some of the errors you're seeing.

@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Jul 14, 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. Fortran Issue involves or relates to Fortran. 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
Development

Successfully merging this pull request may close these issues.

[RFC]: add blas/base/csscal
3 participants