Skip to content

feat: add blas/ext/base/sindex-of #7318

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

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

headlessNode
Copy link
Member


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 ---

Progresses #2656 .

Description

What is the purpose of this pull request?

This pull request:

  • add blas/ext/base/sindexof

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

---
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
---
@stdlib-bot stdlib-bot added the BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). label Jun 11, 2025
@headlessNode headlessNode added Feature Issue or pull request for adding a new feature. GSoC Google Summer of Code. gsoc: 2025 Google Summer of Code (2025). labels Jun 11, 2025
@stdlib-bot
Copy link
Contributor

stdlib-bot commented Jun 11, 2025

Coverage Report

Package Statements Branches Functions Lines
blas/ext/base/sindex-of $\color{green}174/174$
$\color{green}+100.00\%$
$\color{green}12/12$
$\color{green}+100.00\%$
$\color{green}2/2$
$\color{green}+100.00\%$
$\color{green}174/174$
$\color{green}+100.00\%$

The above coverage report was generated for the changes in this PR.

---
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: na
  - task: lint_repl_help
    status: na
  - 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
---
@kgryte kgryte changed the title feat: add blas/ext/base/sindexof feat: add blas/ext/base/sindex-of Jun 12, 2025
@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Jun 12, 2025
---
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: na
  - 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
---
@headlessNode headlessNode marked this pull request as ready for review June 13, 2025 12:26
@stdlib-bot stdlib-bot added the Needs Review A pull request which needs code review. label Jun 13, 2025
@headlessNode headlessNode requested a review from kgryte June 13, 2025 12:26
@headlessNode headlessNode removed the Needs Changes Pull request which needs changes before being merged. label Jun 13, 2025
@kgryte
Copy link
Member

kgryte commented Jun 14, 2025

@headlessNode Would you mind going ahead and adding the C implementation and associated benchmarks, examples, docs, and tests?

The function has the following parameters:

- **N**: number of indexed elements.
- **searchElement**: element to search the index of.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **searchElement**: element to search the index of.
- **searchElement**: search element.

- **N**: number of indexed elements.
- **searchElement**: element to search the index of.
- **x**: input [`Float32Array`][@stdlib/array/float32].
- **strideX**: index increment.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **strideX**: index increment.
- **strideX**: stride length.

- **x**: input [`Float32Array`][@stdlib/array/float32].
- **strideX**: index increment.

If the function is unable to find an element which equals a provided search element, the function returns `-1`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If the function is unable to find an element which equals a provided search element, the function returns `-1`.
If the function is unable to find a search element, the function returns `-1`.

var x = new Float32Array( [ -2.0, 1.0, 3.0, -5.0, 4.0, 0.0, -1.0, 3.0 ] );

var idx = sindexOf( 4, -1.0, x, 2 );
// returns 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. N specifies the view length. Here, that "view" is four elements. 6 is the index in the underlying buffer. That is not what we care about in this case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also blas/base/isamax.


// Find index...
var idx = sindexOf( 3, -6.0, x1, 2 );
// returns 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing. This return value is incorrect.

var x = new Float32Array( [ -2.0, 1.0, 3.0, -5.0, 4.0, 0.0, -1.0, 3.0 ] );

var idx = sindexOf.ndarray( 3, 3.0, x, 1, x.length-3 );
// returns 7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect.


b.tic();
for ( i = 0; i < b.iterations; i++ ) {
out = sindexOf( x.length, 2.0, x, 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
out = sindexOf( x.length, 2.0, x, 1 );
out = sindexOf( x.length, len+1, x, 1 );

2.0 obviously does not make sense here, as then the benchmarks are effectively constant time. You should be benchmarking the worst case scenario (i.e., when you need to search an entire array).

len = pow( 10, i );

f = createBenchmark( len );
bench( pkg+':dtype=float32,len='+len, f );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we including dtype here? Only one dtype is allowed/expected.

Suggested change
bench( pkg+':dtype=float32,len='+len, f );
bench( pkg+':len='+len, f );


b.tic();
for ( i = 0; i < b.iterations; i++ ) {
out = sindexOf( x.length, 2.0, x, 1, 0 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
out = sindexOf( x.length, 2.0, x, 1, 0 );
out = sindexOf( x.length, len+1, x, 1, 0 );

len = pow( 10, i );

f = createBenchmark( len );
bench( pkg+':dtype=float32,len='+len, f );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bench( pkg+':dtype=float32,len='+len, f );
bench( pkg+':len='+len, f );

* Returns the index of a specified search element in a single-precision floating-point strided array.
*
* @param {PositiveInteger} N - number of indexed elements
* @param {Number} searchElement - number to search index of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {Number} searchElement - number to search index of
* @param {number} searchElement - search element

* @param {PositiveInteger} N - number of indexed elements
* @param {Number} searchElement - number to search index of
* @param {Float32Array} x - input array
* @param {integer} strideX - index increment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {integer} strideX - index increment
* @param {integer} strideX - stride length

* Returns the index of a specified search element in a single-precision floating-point strided array using alternative indexing semantics.
*
* @param {PositiveInteger} N - number of indexed elements
* @param {Number} searchElement - number to search index of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {Number} searchElement - number to search index of
* @param {number} searchElement - search element

* @param {PositiveInteger} N - number of indexed elements
* @param {Number} searchElement - number to search index of
* @param {Float32Array} x - input array
* @param {integer} strideX - index increment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {integer} strideX - index increment
* @param {integer} strideX - stride length

ix = offsetX;
for ( i = 0; i < N; i++ ) {
if ( x[ ix ] === searchElement ) {
return ix;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ix;
return i;

t.strictEqual( actual, 3, 'returns expected value' );

actual = sindexOf( x.length, 3.0, x, -1 );
t.strictEqual( actual, 5, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect.

t.strictEqual( actual, 1, 'returns expected value' );

actual = sindexOf( x.length, 2.0, x, -1 );
t.strictEqual( actual, 3, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect.


// Negative stride...
actual = sindexOf( x.length, 1.0, x, -1 );
t.strictEqual( actual, 1, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect.

Comment on lines +53 to +60
actual = sindexOf( x.length, 1.0, x, -1, x.length-1 );
t.strictEqual( actual, 1, 'returns expected value' );

actual = sindexOf( x.length, 2.0, x, -2, x.length-1 );
t.strictEqual( actual, 3, 'returns expected value' );

actual = sindexOf( x.length, 1.0, x, -2, x.length-2 );
t.strictEqual( actual, 0, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all incorrect.

t.strictEqual( actual, 0, 'returns expected value' );

actual = sindexOf( x.length, 2.0, x, 1, 1 );
t.strictEqual( actual, 2, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect.

t.strictEqual( actual, 2, 'returns expected value' );

actual = sindexOf( x.length, 3.0, x, 1, 2 );
t.strictEqual( actual, 4, 'returns expected value' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect.

var ix;
var i;

if ( N <= 0 || x.length === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( N <= 0 || x.length === 0 ) {
if ( N <= 0 ) {

Comment on lines +95 to +102
tape( 'the function returns `-1` if provided an offset which exceeds the maximum array index', function test( t ) {
var actual;

actual = sindexOf( 3, 2.0, new Float32Array( [ 1.0, 2.0, 3.0 ] ), 1, 20 );
t.strictEqual( actual, -1, 'returns expected value' );

t.end();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test should be removed. In strided array implementations, we don't perform buffer bounds checking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. and removed Needs Review A pull request which needs code review. labels Jun 14, 2025
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). Feature Issue or pull request for adding a new feature. GSoC Google Summer of Code. gsoc: 2025 Google Summer of Code (2025). Needs Changes Pull request which needs changes before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants