Skip to content

fix: follow-up fixes for commits merged to develop on 2026-09-28 - #15652

Draft
Planeshifter wants to merge 6 commits into
developfrom
philipp/fix-commit-review-2026-09-29
Draft

Planeshifter wants to merge 6 commits into
developfrom
philipp/fix-commit-review-2026-09-29

Conversation

@Planeshifter

Copy link
Copy Markdown
Member

Follow-up fixes for commits merged to develop between 2026-09-28 20:23 UTC and 2026-09-29 07:03 UTC (e0ab571bc^..32c09fd47, 35 commits, 951 files).

No associated issue.

Description

What is the purpose of this pull request?

This pull request applies follow-up fixes for defects found while reviewing the 35 commits merged to develop in that window. Four issues survived validation; each is fixed in its own per-package commit.

The window covered four themes: a 755-file mechanical consistency pass across 74 blas/ext/base* packages (67c1633c9); four new packages plus three new C implementations under blas/ext; the consensusOrder refactor of the strided1d kernels (100ca4c8d, 32c09fd47); and 11 migrations of test suites to ULP-based assertions. Three of the four fixes are leftovers from the fill-range → fill-between rename and the consistency pass; the fourth is a functional regression in the kernel refactor.

ndarray/base/kernels/generic/{binary,ternary}-strided1d/unblocked

  • Fix consensusOrder call sites in lib/node_modules/@stdlib/ndarray/base/kernels/generic/{binary,ternary}-strided1d/unblocked/lib/{2..10}d.js (18 total): @stdlib/ndarray/base/consensus-order takes a single list of stride arrays, but 100ca4c8d and 32c09fd47 passed them as separate arguments, so only stridesX was inspected and the function always returned 'row-major', leaving the loop-interchange optimization inert and losing the column-major detection. Wrap the stride arrays in a list (consensusOrder( [ stridesX, stridesY, stridesZ ] )), matching the README, and add // eslint-disable-line max-len to the long lines in the ternary 2d-8d kernels.

blas/ext

  • Update the @stdlib/blas/ext namespace declarations and README for the fill-range to fill-between rename missed in 0c2ab03e5: docs/types/index.d.ts (lines 30, 257, 263) still imported the deleted @stdlib/blas/ext/fill-range, which breaks type resolution for the whole namespace, and declared fillRange instead of the exported fillBetween, and README.md (lines 55, 131) kept the old symbol and a dead link. This points the import, member, and JSDoc example at fill-between/fillBetween and corrects the README entry and link definition; alphabetical order is unchanged.

blas/ext/fill-between

  • Rename the FillRange interface to FillBetween in lib/node_modules/@stdlib/blas/ext/fill-between/docs/types/index.d.ts (lines 53 and 333), as 0c2ab03e5 renamed every other identifier but missed it, leaving declare const fillBetween: FillRange; and diverging from siblings such as IndexOfNotEqual; no behavioral change.

blas/ext/base/gindex-of-column, blas/ext/base/gindex-of-row

  • Update the offsetA/offsetX JSDoc in lib/node_modules/@stdlib/blas/ext/base/gindex-of-column/lib/{accessors,base,ndarray}.js and lib/node_modules/@stdlib/blas/ext/base/gindex-of-row/lib/{accessors,base,ndarray}.js from index offset for to starting index for, matching the wording applied in 67c1633c9 to the sibling packages and to these packages' own README.md and docs/types/index.d.ts. The commit converted the adjacent strideA1 lines but missed these 12 lines; docs only.

Related Issues

Does this pull request have any related issues?

No.

Questions

Any questions for reviewers of this pull request?

The consensusOrder fix restores the layout discrimination the two refactor: commits intended, but it does change which traversal branch column-major inputs take. Numeric results are identical either way (verified), so this is a performance-path change rather than a correctness one — worth a second look if the positional-argument form was deliberate.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Validation

  • Style guide compliance. Changed and newly added packages were compared against established reference packages of similar complexity in the same family (first-index-less-than vs. index-of-not-equal; gfind-index-between vs. gfind-index; gcusome vs. gcuany; the new C scaffolds vs. dwxsa/done-to), checking license headers, JSDoc completeness, @module/@example paths, package.json fields, C include guards, and alphabetical ordering in namespace lib/index.js, index.d.ts, and README tables of contents.
  • Bug scan. Two independent passes over the diff: one restricted to the diff text, one permitted to consult reference code. Covered off-by-one and index/stride arithmetic, wrong-variable substitutions in the near-identical 0d–10d kernels, dtype mix-ups across the c/d/s/z/g variants, C memory safety and header/src/addon/binding.gyp signature parity, and the ULP migrations (operand order, retained expected values, orphaned EPS/delta/tol variables and requires).
  • Verification of the consensusOrder fix. consensus-order was exercised directly to confirm the positional form returns 'row-major' for every input, including column-major strides, while the list form discriminates correctly and honours the majority. Both kernels were then driven end-to-end across shapes [3,2], [2,3,2], [2,2,3,2], and [2,2,2,3,2] in both layouts (16 cases): all pass, and results are identical before and after the fix, confirming the defect is a disabled optimization rather than incorrect output.
  • Lint. The repo's max-len settings (code: 80, tabWidth: 4) were applied to all 18 changed kernel files: zero errors, and no redundant eslint-disable directives introduced.

Deliberately excluded

  • Anything requiring interpretation or a judgment call, and anything that could not be validated without changes outside the window's diff.
  • Completing the ... → : comment-punctuation migration in the blas/ext/base* READMEs. 67c1633c9 converted 81 such comments but left 79 in the ellipsis form and added two new ones, so the ellipsis form is not being treated as incorrect; finishing that pass is a separate mechanical change.
  • Missing index.d.ts and README table-of-contents entries for gcusome, gfindIndexBetween, and gindexOfGreaterThanSorted. These symbols were registered in the JS namespaces after the corresponding declaration commits, so the entries lag by one commit. Unlike the fill-range import, these are absent entries rather than dangling references, and adding them falls outside the window's diff.
  • The max( 1, s ) → s change to the max(1,%d) RangeError messages in four *index-of-column/*last-index-of-row packages. The template contains the literal text max(1,%d), so the placeholder is the operand, not the result; passing max( 1, s ) double-applied it. The change is correct, max remains used in the guard, and the pass is complete across all 89 files carrying that message.

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

This PR was written by Claude Code as an automated review of the commits merged to develop in the stated window. Candidate issues were produced by four independent reviewer passes, then each surviving issue was re-verified against the working tree before any edit was made; the consensusOrder defect and its fix were additionally confirmed by executing the affected kernels. No human has audited these changes yet, so the PR is opened as a draft.


@stdlib-js/reviewers


Generated by Claude Code

…nary-strided1d/unblocked`

`ndarray/base/consensus-order` accepts a single argument: a list of stride
arrays. The kernels passed the stride arrays as separate positional
arguments, so only `stridesX` was inspected and its elements (plain
numbers) were fed to `strides2order`, which reports "disorganized" for
each. The tally therefore always fell through to the default layout and
`isRowMajor` was unconditionally `true`, leaving the consensus
uncomputed and the previous column-major detection lost. Results were
unaffected, but the loop interchange was pessimal for column-major
input.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
…rnary-strided1d/unblocked`

`ndarray/base/consensus-order` accepts a single argument: a list of stride
arrays. The kernels passed the stride arrays as separate positional
arguments, so only `stridesX` was inspected and its elements (plain
numbers) were fed to `strides2order`, which reports "disorganized" for
each. The tally therefore always fell through to the default layout and
`isRowMajor` was unconditionally `true`, leaving the consensus
uncomputed and the previous column-major detection lost. Results were
unaffected, but the loop interchange was pessimal for column-major
input.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
The rename to `fill-between` updated `lib/index.js` and deleted the old
package, but left the namespace declarations importing
`@stdlib/blas/ext/fill-range`, which no longer resolves, and declaring
`fillRange`, which is no longer exported. The README table of contents
kept the old symbol and a dead link.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
The rename left the declaration interface named `FillRange`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
…of-column`

The consistency pass converted `index offset for` to
`starting index for` in the sibling row and column search packages and
in this package's README and declarations, but not in its JSDoc.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
…of-row`

The consistency pass converted `index offset for` to
`starting index for` in the sibling row and column search packages and
in this package's README and declarations, but not in its JSDoc.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BNoKQHbVCqA5SsC9A1UBgd
@stdlib-bot

Copy link
Copy Markdown
Contributor

Coverage Report

Package Statements Branches Functions Lines
blas/ext $\\color{red}170742/207679$
$\\color{green}+82.21\\%$
$\\color{red}2980/3324$
$\\color{green}+89.65\\%$
$\\color{red}20/1671$
$\\color{green}+1.20\\%$
$\\color{red}170742/207679$
$\\color{green}+82.21\\%$
blas/ext/base/gindex-of-column $\\color{green}554/554$
$\\color{green}+100.00\\%$
$\\color{green}55/55$
$\\color{green}+100.00\\%$
$\\color{green}4/4$
$\\color{green}+100.00\\%$
$\\color{green}554/554$
$\\color{green}+100.00\\%$
blas/ext/base/gindex-of-row $\\color{red}470/554$
$\\color{green}+84.84\\%$
$\\color{red}36/37$
$\\color{green}+97.30\\%$
$\\color{red}3/4$
$\\color{green}+75.00\\%$
$\\color{red}470/554$
$\\color{green}+84.84\\%$
blas/ext/fill-between $\\color{green}484/484$
$\\color{green}+100.00\\%$
$\\color{green}55/55$
$\\color{green}+100.00\\%$
$\\color{green}2/2$
$\\color{green}+100.00\\%$
$\\color{green}484/484$
$\\color{green}+100.00\\%$
ndarray/base/kernels/generic/binary-strided1d/unblocked $\\color{red}2132/3568$
$\\color{green}+59.75\\%$
$\\color{green}13/13$
$\\color{green}+100.00\\%$
$\\color{red}0/12$
$\\color{green}+0.00\\%$
$\\color{red}2132/3568$
$\\color{green}+59.75\\%$
ndarray/base/kernels/generic/ternary-strided1d/unblocked $\\color{red}2419/3975$
$\\color{green}+60.86\\%$
$\\color{green}13/13$
$\\color{green}+100.00\\%$
$\\color{red}0/12$
$\\color{green}+0.00\\%$
$\\color{red}2419/3975$
$\\color{green}+60.86\\%$

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

This branch has not been deployed

No deployments
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 this pull request may close these issues.

3 participants