feat: add blas/ext/base/gconjoin#11142
feat: add blas/ext/base/gconjoin#11142headlessNode wants to merge 3 commits intostdlib-js:developfrom
blas/ext/base/gconjoin#11142Conversation
Coverage Report
The above coverage report was generated for the changes in this PR. |
lib/node_modules/@stdlib/blas/ext/base/gconjoin/docs/types/index.d.ts
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/lib/accessors.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/lib/accessors.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/test/test.main.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/test/test.main.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/test/test.ndarray.js
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/blas/ext/base/gconjoin/test/test.ndarray.js
Outdated
Show resolved
Hide resolved
| if ( N <= 0 ) { | ||
| return ''; | ||
| } | ||
| o = arraylike2object( x ); |
There was a problem hiding this comment.
When conjunction is the empty string, we should delegate to blas/ext/base/gjoin which has a fast path and method delegation.
if ( conjunction === '' ) {
return prefix + gjoin( N, SEP, x, strideX, offsetX ) + suffix;
}
There was a problem hiding this comment.
Actually, you could more generally do a fast path when the stride is 1. When no conjunction, delegate fully to gjoin. When there is a conjunction, use gjoin for the first N-1 elements and then handle the last element as you do below.
This should impact your last element handling logic below as the no-conjunction case can be handled via gjoin with final concat with prefix and suffix.
|
For METR, |
Co-authored-by: Athan <kgryte@gmail.com> Signed-off-by: Athan <kgryte@gmail.com>
| if ( strideX === 1 ) { | ||
| out = prefix + gjoin( N-1, SEP, x, strideX, offsetX ); | ||
| } else { | ||
| ix = offsetX; | ||
| out = prefix + str( x, ix ); | ||
| ix += strideX; | ||
| for ( i = 1; i < N - 1; i++ ) { | ||
| out += SEP + str( x, ix ); | ||
| ix += strideX; | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at this, do we actually need to have a special path for strideX == 1?
| if ( strideX === 1 ) { | |
| out = prefix + gjoin( N-1, SEP, x, strideX, offsetX ); | |
| } else { | |
| ix = offsetX; | |
| out = prefix + str( x, ix ); | |
| ix += strideX; | |
| for ( i = 1; i < N - 1; i++ ) { | |
| out += SEP + str( x, ix ); | |
| ix += strideX; | |
| } | |
| } | |
| out = prefix + gjoin( N-1, SEP, x, strideX, offsetX ); |
| o = arraylike2object( x ); | ||
| if ( o.accessorProtocol ) { | ||
| return accessors( N, prefix, suffix, conjunction, oxfordComma, o, strideX, offsetX ); // eslint-disable-line max-len | ||
| } |
There was a problem hiding this comment.
I am no longer sure it is actually necessary anymore to have a dedicated accessors.js file.
If conjunction is '' or N == 1, we can delegate fully to gjoin.
For the first N-1 elements, we can delegate to gjoin.
It is only for the last element that we need specialized behavior. At which point, I think it is fine if instead of arraylike2object, we use array/base/resolve-getter on the input array x and use the str function from the accessors.js file to resolve the last element.
So I think we can streamline the implementation a bit further.
kgryte
left a comment
There was a problem hiding this comment.
Apart from refactoring the implementation, this is coming along.
|
For METR, |
Resolves stdlib-js/metr-issue-tracker#209.
Description
This pull request:
blas/ext/base/gconjoinRelated Issues
This pull request has the following related issues:
blas/ext/base/gconjoinmetr-issue-tracker#209Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
Primarily written by Claude Code.
@stdlib-js/reviewers