DGemm with offset

[Quafadas]

Fixes #23

Full disclosure: I delegated this to copilot.

I'm not familiar with the netlib codebase, so I can't say whether or not this is in the right style, but the fix looks superficially the way I would have expected. Copilot added and ran extra tests, which reproduce and fix the case I reported.

Copilot also claimed to have checked for other regressions.

I'm hopeful, that the CI will tell us if anything has regressed.

I do note however, that this makes the offset checks strictly looser.

Loosening the offset conditions in this manner, would appear to open up a clutch of new code paths, which are not tested by this PR beyond this single naive test. This change may therefore be risky in terms of a preference to throw an error in preference to producing incorrect numbers. Feedback welcomed.

[Copilot]

Pull Request Overview

This PR fixes incorrect bounds-checking calculations for offset-access in SGEMM, DGEMM, SGEMV, and DGEMV by adjusting the checkIndex formulas to use the precise last-element index rather than a loose multiplication-based bound. It also adds parameterized tests to reproduce and verify the fix for each of these routines.

• Refined checkIndex logic in AbstractBLAS.java for both single- and double-precision GEMM and GEMV
• Added testOffsetBoundsChecking tests in the four corresponding test classes
• Ensures no IndexOutOfBoundsException is thrown for valid offsets and matches results against the F2J reference implementation

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
AbstractBLAS.java	Updated checkIndex formulas for SGEMM, DGEMM, SGEMV, DGEMV
SgemvTest.java	Added testOffsetBoundsChecking for SGEMV
DgemvTest.java	Added testOffsetBoundsChecking for DGEMV
SgemmTest.java	Added testOffsetBoundsChecking for SGEMM
DgemmTest.java	Added testOffsetBoundsChecking for DGEMM

Comments suppressed due to low confidence (3)

blas/src/test/java/dev/ludovic/netlib/blas/SgemvTest.java:109

• The new test only covers the non-transposed ('N') case; consider adding a similar testOffsetBoundsChecking invocation with trans="T" to ensure the offset logic works for the transpose branch as well.

            blas.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, y, 0, 1);

blas/src/test/java/dev/ludovic/netlib/blas/DgemmTest.java:192

• [nitpick] For consistency with the other test classes, you might clarify this comment to read "// Test case that reproduces the original bounds checking issue for dgemm".

        // Test case that reproduces the original bounds checking issue

blas/src/main/java/dev/ludovic/netlib/blas/AbstractBLAS.java:294

• [nitpick] Consider applying the same precise offset-bound check adjustments to the complex routines (cgemm/zgemm and cgemv/zgemv) so that all BLAS variants remain consistent.

    checkIndex(offseta + (lsame("N", transa) ? (k - 1) * lda + (m - 1) : (m - 1) * lda + (k - 1)), a.length);

[luhenry]

It's surprising the change would work as the test does try to fix the use of offseta without changing the usage of offseta. CoPilot probably got confused about Fortran's use of 1-indexed arrays when Java is 0-indexed (arr[1] points to the first element in Fortran, but the second element in Java or C). This seems to work with the provided tests. Can you please rebase your change, and add a offset=2 test to make sure it's not a off-by-1 error still?

I'll need to update some dependencies to run CI though, let me do that first.

[luhenry]

@Quafadas I merged your changes with some minor modifications. Thanks for the contribution! Very happy to review some more if you are interested :)

[Quafadas]

@luhenry

thanks for the review / merge / release :-)

I’m afk for a couple weeks, but still intending to keep exploring netlib. I have the hope, that I will not find more things :-)! But if I do I’ll see if I can follow a similar pattern to the way this issue panned out.