Skip to content

feat: add fromIndex support to blas/ext/base/ndarray/gfind-last-index - #14479

Open
headlessNode wants to merge 1 commit into
stdlib-js:developfrom
headlessNode:blas/ref/gfind-last-index
Open

feat: add fromIndex support to blas/ext/base/ndarray/gfind-last-index#14479
headlessNode wants to merge 1 commit into
stdlib-js:developfrom
headlessNode:blas/ref/gfind-last-index

Conversation

@headlessNode

Copy link
Copy Markdown
Member

Resolves stdlib-js/metr-issue-tracker#1183.

Description

What is the purpose of this pull request?

This pull request:

  • add fromIndex support to blas/ext/base/ndarray/gfind-last-index

Related Issues

Does this pull request have any related issues?

This pull request has the following related issues:

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.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

  • Yes
  • No

If you answered "yes" above, how did you use AI assistance?

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Disclosure

If you answered "yes" to using AI assistance, please provide a short disclosure indicating how you used AI assistance. This helps reviewers determine how much scrutiny to apply when reviewing your contribution. Example disclosures: "This PR was written primarily by Claude Code." or "I consulted ChatGPT to understand the codebase, but the proposed changes were fully authored manually by myself.".

Primarily written by Claude Code.


@stdlib-js/reviewers

@headlessNode
headlessNode requested a review from a team August 21, 2026 13:15
@stdlib-bot stdlib-bot added BLAS Issue or pull request related to Basic Linear Algebra Subprograms (BLAS). Needs Review A pull request which needs code review. labels Aug 21, 2026
@headlessNode headlessNode added Feature Issue or pull request for adding a new feature. METR Pull request associated with the METR project. labels Aug 21, 2026
@stdlib-bot

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
blas/ext/base/ndarray/gfind-last-index $\\color{green}151/151$
$\\color{green}+100.00\\%$
$\\color{green}5/5$
$\\color{green}+100.00\\%$
$\\color{green}2/2$
$\\color{green}+100.00\\%$
$\\color{green}151/151$
$\\color{green}+100.00\\%$

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

@headlessNode

Copy link
Copy Markdown
Member Author

The test failure is from top-level blas/ext/find-last-index. It would be updated after this PR is merged.

@kgryte

kgryte commented Aug 21, 2026

Copy link
Copy Markdown
Member

Do we want fromIndex support in *find*? The Array.prototype.findLastIndex does not support a fromIndex: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findLastIndex

@kgryte

kgryte commented Aug 21, 2026

Copy link
Copy Markdown
Member

...that said, it is not clear why the standard bodies did not include fromIndex support.

@kgryte

kgryte commented Aug 21, 2026

Copy link
Copy Markdown
Member

Update: did a bit of digging. find and findIndex were added to ES2015, so their reasoning is not easily publicly available, but the gist, as best I can infer, is that, for callback-based APIs, they saw imposing a fromIndex constraint as a matter of composition, given that the element index is passed as an argument to the provided callback.

function predicate( v, i ) {
	if ( i < fromIndex ) {
		return false;
	}
	// ...
}

This also made it consistent with the other callback-based APIs (e.g., there is no fromIndex argument in map).

I do, however, think there is an argument that we should add support for a fromIndex argument if we consider that find et al really belong to a family of "search" APIs and thus want to prioritize consistency within those APIs. I am sympathetic to that argument, especially as it means users can avoid needless iteration over regions of an ndarray dimension.

Furthermore, if we support a fromIndex, it can be broadcasted and users can supply different from indices to apply to different rows or columns, which would be much more complicated if users had to use callbacks alone.

TL;DR: I am on board with adding fromIndex support.

@kgryte kgryte added release: Major Breaking change requiring a new major release. review: 5 and removed Needs Review A pull request which needs code review. labels Aug 22, 2026
'dtype': 'generic'
});
actual = gfindLastIndex( [ x, fromIndex ], isEven );
t.strictEqual( actual, 0, 'returns expected value' );

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This doesn't seem correct. If I provide -7 for a six element array, I have exceeded the beginning of the array. But here, you are clamping to 0, resulting in a valid return value; however, we should be returning -1.

This ensures parity with gfind-index. There, if a positive fromIndex exceeds the array bounds, we always return -1. We should behave similarly here.

This is likely going to apply to all the "last" packages you have recently refactored and will need further correction.

Accordingly, this brings me back to ndarray/base/clip-upper-index, where the lower bound is not clamped. Your logic in the wrapper would then need to change such that if the returned value is less than zero, you return -1.


N = numelDimension( x, 0 );
fromIndex = clipIndex( fromIndex, N );
if ( fromIndex >= N ) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this check necessary? Instead, for L75, you should use N-1. Then, you don't need to explicitly set to N-1. This is going to apply to your other "last" refactorings and should be addressed in a follow-up PR for those packages.

@kgryte kgryte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left initial comments.

@kgryte kgryte added Needs Changes Pull request which needs changes before being merged. review: 3 and removed review: 5 labels Aug 22, 2026
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. METR Pull request associated with the METR project. Needs Changes Pull request which needs changes before being merged. release: Major Breaking change requiring a new major release. review: 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC]: add fromIndex parameter support to blas/ext/base/ndarray/gfind-last-index

3 participants