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

[Copilot]

[nitpick] The test calls sgemv once inside assertDoesNotThrow and then again to verify the result; you could simplify by capturing the output in a single invocation and asserting no exception while also checking the returned vector.

Suggested change

	assertDoesNotThrow(() -> {
	blas.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, y, 0, 1);
	});
	// Compare with F2J result
	f2j.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, yExpected, 0, 1);
	blas.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, y, 0, 1);
	try {
	blas.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, y, 0, 1);
	} catch (Exception e) {
	fail("sgemv threw an unexpected exception: " + e.getMessage());
	}
	// Compare with F2J result
	f2j.sgemv("N", 2, 3, 1.0f, a, 1, 3, x, 0, 1, 0.0f, yExpected, 0, 1);