Skip to content
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

fix for #850: transpose-slice-multiply crash #862

Merged

Conversation

philwalk
Copy link
Contributor

@philwalk philwalk commented Aug 28, 2024

The fix consisted of changing the comparison operator from < to > here:
diff math/src/main/codegen/breeze/linalg/operators/DenseMatrixOps.scala

       // if we have a weird stride...
       val a: DenseMatrix[Double] =
-        if (_a.majorStride < math.max(if (_a.isTranspose) _a.cols else _a.rows, 1)) _a.copy else _a
+        if (_a.majorStride > math.max(if (_a.isTranspose) _a.cols else _a.rows, 1)) _a.copy else _a
       val b: DenseMatrix[Double] =
-        if (_b.majorStride < math.max(if (_b.isTranspose) _b.cols else _b.rows, 1)) _b.copy else _b
+        if (_b.majorStride > math.max(if (_b.isTranspose) _b.cols else _b.rows, 1)) _b.copy else _b

The problem occurs when majorStride is too big rather than too small, so a copy is needed.
This might represent a performance improvement, on average, if it results in fewer calls to _.copy.

added a test to math/src/test/scala/breeze/linalg/DenseMatrixTest.scala

+  test("#850 - transpose slice multiply bug") {
+    val  = DenseMatrix( (0.75,  -0.25), (-0.25,  0.75))
+    val matB = DenseMatrix(
+      (67.0, 33.0),
+      (69.0, 78.0),
+      (93.0, 57.0)
+    ).t
+    val B = matB(::, 1 until matB.cols)
+    Jɴ * B      // should not crash
+  }

Also fixed a test that was failing in Windows due to a hardwired / path separator: math/src/test/scala/breeze/linalg/TextOperationsTest.scala
Fixed it by converting \\ to '/' to find the correct relative path to the resource file.

Upgraded the sbt version because sbt was crashing.

Upgraded scala versions.

diff --git a/project/Build.scala b/project/Build.scala
-  val buildCrossScalaVersions = Seq("3.1.3", "2.12.15", "2.13.8")
+  val buildCrossScalaVersions = Seq("3.4.2", "2.12.19", "2.13.14")

One test was failing on my system before I made any modifications, and is still failing now, so not sure what that's about.

[info] ProjectedQuasiNewtonTest:
[info] - optimize a simple multivariate gaussian (366 milliseconds)
[info] - optimize a simple multivariate gaussian with projection (136 milliseconds)
[info] - optimize a simple multivariate gaussian with l2 regularization (75 milliseconds)
[info] - optimize a complicated function without projection (393 milliseconds)
[info] - simple linear solve without projection (50 milliseconds)
L1 projection iter 67
[info] - simple linear solve with L1 projection compared to Octave *** FAILED *** (134 milliseconds)
[info]   1.1197254381828163E-4 was not less than 1.0E-4 (ProjectedQuasiNewtonTest.scala:221)

@dlwh dlwh merged commit 1189911 into scalanlp:master Aug 29, 2024
@dlwh
Copy link
Member

dlwh commented Aug 29, 2024

thanks!

@philwalk philwalk deleted the fix-for-#850-multiply-transpose-slice-crash branch August 29, 2024 12:57
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.

2 participants