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

[RFC]: add blas/base/grot #2190

Open
3 tasks done
performant23 opened this issue Apr 19, 2024 · 33 comments · May be fixed by #2195
Open
3 tasks done

[RFC]: add blas/base/grot #2190

performant23 opened this issue Apr 19, 2024 · 33 comments · May be fixed by #2195

Comments

@performant23
Copy link
Contributor

Description

This RFC proposes adding the generic JavaScript interface grot to apply plane rotation on input arrays of any type.

Related Issues

N/A

Questions

No.

Other

No.

Checklist

  • I have read and understood the Code of Conduct.
  • Searched for existing issues and pull requests.
  • The issue name begins with RFC:.
@performant23
Copy link
Contributor Author

Hi @kgryte, I was just testing the implementation and was encountering some errors for the tests pertaining to the accessor arrays. I checked the logs on the implementation at lib/main.js and for the below testcase, got ox and oy as:

tape( 'the function supports an `x` stride (accessors)', function test( t ) {
	var xe;
	var ye;
	var x;
	var y;

	x = new Complex128Array([
		1.0, // 0
		2.0,
		3.0, // 1
		4.0
	]);
	y = new Complex128Array([
		6.0, // 0
		7.0, // 1
		8.0,
		9.0
	]);

	grot( 2, x, 2, y, 1, 0.8, 0.6 );

	xe = new Complex128Array ( [ 4.4, 2.0, 6.6, 4.0 ] );
	ye = new Complex128Array( [ 4.2, 3.8, 8.0, 9.0 ] );

	isApprox( t, reinterpret128( x, 0 ), reinterpret128( xe, 0 ), 2.0 );
	isApprox( t, reinterpret128( y, 0 ), reinterpret128( ye, 0 ), 2.0 );

	t.end();
});
{
  data: Complex128Array {},
  dtype: 'complex128',
  accessorProtocol: false,
  accessors: [ [Function: getArrayLike], [Function: setArrayLike] ]
}
{
  data: Complex128Array {},
  dtype: 'complex128',
  accessorProtocol: false,
  accessors: [ [Function: getArrayLike], [Function: setArrayLike] ]
}

This means that basically when the arguments are passed onto the function, it isn't calling the implementation in accessors.js but instead executing operations intended for numeric arrays.

I checked other implementations (gswap and gcopy) which had accessor array support but I got the same logs and the tests aimed at the implementation for the accessor arrays are failing.

So, could you please guide me on calling and testing the accessor array implementation? Thanks!

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

@performant23 That's a bit odd, as when I run the tests for gcopy, I get

the function supports an `x` stride (accessors)

    {
      data: Complex128Array {},
      dtype: 'complex128',
      accessorProtocol: true,
      accessors: [ [Function: getComplex128], [Function: setComplex128] ]
    } {
      data: Complex128Array {},
      dtype: 'complex128',
      accessorProtocol: true,
      accessors: [ [Function: getComplex128], [Function: setComplex128] ]
    }
    ✔ returns expected value

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

I'd need to see your complete test file to be able to debug.

@performant23
Copy link
Contributor Author

Yeah, strange indeed! When I ran gcopy tests, got this:

image

@performant23
Copy link
Contributor Author

I'd need to see your complete test file to be able to debug.

Yeah I can open up a draft PR for this!

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Hmm...can you tell me about bit more about how you are running the tests? Also, in the stdlib REPL, run

In [1]: var arraylike2object = require( '@stdlib/array/base/arraylike2object' );

In [2]: arraylike2object( new Complex128Array( [ 1.0, 2.0, 3.0, 4.0 ] ) )
???

@performant23
Copy link
Contributor Author

So, I used the usual test filter i.e. $ make TESTS_FILTER=".*/blas/base/gcopy/.*" test.

Will get back to you regarding the REPL run.

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Also, what version of Node.js are you running?

@performant23
Copy link
Contributor Author

My Node.js version is v18.16.0

Also, here's the result from the REPL run:

image

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Whoa. That is bizarre.

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

In the REPL, do

var isAccessorArray = require( '@stdlib/array/base/assert/is-accessor-array' );

var z = new Complex128Array( [ 1.0, 2.0 ] );

isAccessorArray( z )

typeof z.get

typeof z.set

@performant23
Copy link
Contributor Author

Yeah, this test gives a positive:

In [1]: var isAccessorArray = require( '@stdlib/array/base/assert/is-accessor-array' );

In [2]: var z = new Complex128Array( [ 1.0, 2.0 ] );

In [3]: isAccessorArray( z )
Out[3]: true

In [4]: typeof z.get
Out[4]: 'function'

In [5]: typeof z.set
Out[5]: 'function'

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Okay, that is weird. As isAccessorArray is what is used by arraylike2object.

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Can you also run from the terminal

$ make test TESTS_FILTER=".*/array/base/arraylike2object/.*"

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

array/base/arraylike2object has an explicit test for complex number arrays.

@performant23
Copy link
Contributor Author

yeah, sure!

@performant23
Copy link
Contributor Author

Got all tests passing except the data buffer accessors one:

image

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

In the source code of arraylike2object, log the values of x and the result of isAccessorArray( x ) (L55-56), and run the tests again.

@performant23
Copy link
Contributor Author

performant23 commented Apr 19, 2024

It logged (additionally, dt is: complex64):

  the function converts an array to a standardized object (data buffer accessors)

    x:  Complex64Array {}
    isAccessorArray(x): false
    √ returns expected value
    √ returns expected value

    × returns expected value
    -------------------------
      operator: equal
      expected: true
      actual:   false
      at: process.processImmediate (node:internal/timers:476:21)

    √ returns expected value
    √ returns expected value
    √ returns expected value
    √ returns expected value
    √ returns expected value
    √ returns expected value

@performant23
Copy link
Contributor Author

performant23 commented Apr 19, 2024

Also, logging the same test value for lib/main.js of is-accessor-array is giving false to me:

var x = new Complex64Array( 10 );
console.log(isAccessorArray(x));

Additionally,
logging Array.isArray(value.accessors) is giving false and logging typeof value.accessors[0] is giving this message (Since the previous evaluation is false):

d:\stdlib\lib\node_modules\@stdlib\array\base\assert\is-accessor-array\lib\main.js:46
	console.log("typeof value.accessors[0]", typeof value.accessors[0]);
	                                                               ^

TypeError: Cannot read properties of undefined (reading '0')
    at isAccessorArray (d:\stdlib\lib\node_modules\@stdlib\array\base\assert\is-accessor-array\lib\main.js:46:65)
    at Object.<anonymous> (d:\stdlib\lib\node_modules\@stdlib\array\base\assert\is-accessor-array\lib\main.js:56:13)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.16.0

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Wait...why are you logging value.accessors? Where is this happening?

@performant23
Copy link
Contributor Author

performant23 commented Apr 20, 2024

This is resolved, thank you @kgryte! I have fixed the implementation. Also, just to confirm, accessor tests are mainly testing the stride support (x, y, negative) and the main implementation working. I have finished the tests for strides. Since for drot as per recent changes, we have bifurcated the main implementation tests into 4 cases for varying (sx, sy) as (1, 1), (2,-2), (-2, 1), and (-1, -2), we'd need the accessor tests for all of those cases right?

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

Yes, that would probably be best.

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

You use convert a normal array to an accessor array using @stdlib/array/base/to-accessor-array.

@performant23
Copy link
Contributor Author

performant23 commented Apr 20, 2024

You use convert a normal array to an accessor array using @stdlib/array/base/to-accessor-array.

So, the current tests for grot and also gcopy, gswap use Complex128 for accessor tests. And to-accessor-array leaves the array unchanged for the arrays with such data types ("If the provided array-like object already supports the accessor protocol, the function returns the input array unchanged."). In this case, we won't need to pass to-accessor-array right?

(Hope you don't mind me being pedantic :), just wanted to keep things as clear as possible)

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

Yes, but you'll want to add separate complex array tests, as the algorithm, IIRC, is different depending on whether real or complex.

@performant23
Copy link
Contributor Author

Edit: Now that I think about it, maybe we shouldn't use Complex128 for the accessor tests actually cause if we do, we have to set the values for x and y in the form: new Complex 128 ( real, imaginary ) always and so, our output return type remains fixed at Complex128() for any accessor array which we might not want. A counterexample would be converting a numeric array to an accessor array which won't have any real or imaginary components.

But what if Complex128Array is tested against the accessor array implementation? Since to-accessor-array returns the same array back and we pass it to accessors.js, for complex numbers, the result has to be returned in the form of Complex128Array.

To elaborate, if we have Complex128Array, we'd need to use @stdlib/complex/real and @stdlib/complex/imag' to perform rotations on the complex numbers for each parts seperately. Something like:

	for ( i = 0; i < N; i++ ) {
		tmp = new Complex128(  (  (  c * real( get( xbuf, ix ) ) ) + ( s * real( get(ybuf, iy ) ) ) ), ( ( c * imag( get(xbuf, ix ) ) ) + ( s * imag( get(ybuf, iy ) ) ) ) );
		set( ybuf, iy, new Complex128( ( ( c * real( get(ybuf, iy ) ) ) - ( s * real( get(xbuf, ix ) ) ) ), ( ( c * imag( get(ybuf, iy ) ) ) - ( s * imag( get(xbuf, ix ) ) ) ) ) );
		set( xbuf, ix, tmp );
		ix += strideX;
		iy += strideY;
	}

But this won't work for numeric arrays converted to accessor arrays using to-accessor-array since the function would return something like

AccessorArray {
  _buffer: [ 1, 2, 3 ],
  _getter: [Function: getGeneric],
  _setter: [Function: setGeneric]
}

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

For complex arrays, you need to conjugate and use complex number arithmetic. That will likely entail a separate internal implementation than a plain accessor array where we can assume real values.

@performant23
Copy link
Contributor Author

Yes, but you'll want to add separate complex array tests, as the algorithm, IIRC, is different depending on whether real or complex.

oh, yeah right this is what I wanted to say

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

bifurcated the main implementation tests into 4 cases for varying (sx, sy) as (1, 1), (2,-2), (-2, 1), and (-1, -2)

Note that the test cases in drot for these particular variants were pulled directly from the reference BLAS test suite. You'll need to tailor for complex arrays by consulting the same test suite.

@performant23
Copy link
Contributor Author

Right, so we'd need to modify our accessors.js implementation to first check whether we have a real or a complex array, and then apply different implementations subsequently.

@kgryte
Copy link
Member

kgryte commented Apr 20, 2024

That's right. An example of the idea:

@performant23
Copy link
Contributor Author

Ah, got it! That implementation makes things really clear now!

@performant23 performant23 linked a pull request Apr 21, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants