feat: add blas/ext/join-between#11287
Conversation
---
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes.
report:
- task: lint_filenames
status: passed
- task: lint_editorconfig
status: passed
- task: lint_markdown
status: passed
- task: lint_package_json
status: passed
- task: lint_repl_help
status: passed
- task: lint_javascript_src
status: passed
- task: lint_javascript_cli
status: na
- task: lint_javascript_examples
status: passed
- task: lint_javascript_tests
status: passed
- task: lint_javascript_benchmarks
status: passed
- task: lint_python
status: na
- task: lint_r
status: na
- task: lint_c_src
status: na
- task: lint_c_examples
status: na
- task: lint_c_benchmarks
status: na
- task: lint_c_tests_fixtures
status: na
- task: lint_shell
status: na
- task: lint_typescript_declarations
status: passed
- task: lint_typescript_tests
status: passed
- task: lint_license_headers
status: passed
---
Coverage Report
The above coverage report was generated for the changes in this PR. |
| * @param {string} [options.prefix=''] - prefix to prepend to each joined string | ||
| * @param {string} [options.suffix=''] - suffix to append to each joined string |
There was a problem hiding this comment.
These should both be allowed to be ndarrays. What if I want to prefix each joined string with different prefixes? Ditto for suffix.
| if ( opts.separators.length === 1 ) { | ||
| separators = broadcastScalar( opts.separators[ 0 ], 'generic', [ 1 ], order ); | ||
| } else { | ||
| separators = new ndarray( 'generic', opts.separators, [ opts.separators.length ], [ 1 ], 0, order ); | ||
| } |
There was a problem hiding this comment.
What's going on here? Why isn't separators being broadcast against the entire input array x? What if I want different separators for each row in a matrix?
| separators = new ndarray( 'generic', opts.separators, [ opts.separators.length ], [ 1 ], 0, order ); | ||
| } | ||
| // Perform the reduction: | ||
| unaryReduceStrided1d( dispatch, [ x, y ], opts.dims ); |
There was a problem hiding this comment.
This is odd. Why are you doing it this way? A closure should not be necessary if you have done broadcasting correctly.
There was a problem hiding this comment.
Using unaryReduceStrided1d and not *-dispatch and not -factory skips a bunch of validation logic that would have to be reimplemented here. That is not desirable. Ideally, we'd figure out a way to use the *-factory package.
There was a problem hiding this comment.
Validation including acceptable dtypes. TL;DR: there is a lot currently incorrect with this implementation.
| * @param {Array<Object>} arrays - ndarrays | ||
| * @returns {string} joined string | ||
| */ | ||
| function dispatch( arrays ) { |
There was a problem hiding this comment.
This function should not be necessary. prefix, suffix, and separators should all be passed down as auxiliary arrays via unaryReduceStrided1d, not via a closure like this.
There was a problem hiding this comment.
Hmm...but I see that separators will be problematic here, as unaryReduceStrided1d assumes all ancillary arrays are 0D, while separators will always be 1D.
There was a problem hiding this comment.
It's possible that we need only tweak unary-reduce-strided1d slightly here.
There was a problem hiding this comment.
@kgryte I think two changes in unary-reduce-strided1d would unlock the factory approach:
- Allow arrays with extra trailing dimensions here: https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/ndarray/base/unary-reduce-strided1d/lib/main.js#L380
- Pass
Khere: https://github.com/stdlib-js/stdlib/blob/develop/lib/node_modules/%40stdlib/ndarray/base/unary-reduce-strided1d/lib/main.js#L428, and for array with more thanKdimensions, preserved trailing dimensions instead of collapsing to0d.
I believe the iteration code would work as is. Should I go ahead with these changes in unary-reduce-strided1d? and the binary-* counterpart as well?
type: pre_commit_static_analysis_report
description: Results of running static analysis checks when committing changes. report:
Resolves stdlib-js/metr-issue-tracker#238.
Description
This pull request:
blas/ext/join-betweenRelated Issues
This pull request has the following related issues:
blas/ext/join-betweenmetr-issue-tracker#238Questions
No.
Other
No.
Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
Primarily written by Claude Code but with heavy back-and-forth fixing.
@stdlib-js/reviewers