JetBrains-Research / viktor

Efficient f64-only ndarray in Kotlin
MIT License
125 stars 6 forks source link

isDense property is inaccurate #19

Closed dievsky closed 5 years ago

dievsky commented 5 years ago

It says so right in the comment:

// This is inaccurate, but maybe sufficient for our use-case?

It's not the best practice to rely on such assumptions. In fact, they can be easily violated using the provided API only.

val a = DoubleArray(8) { it.toDouble() }.asF64Array().reshape(2, 2, 2) // a 2x2x2 3D-array
val aView0 = a.view(0, 1) // a 2D-subarray with Y = 0
val aView1 = a.view(1, 1) // a 2D-subarray with Y = 1
assertEquals(F64Array.of(0.0, 1.0, 4.0, 5.0).reshape(2, 2), aView0)
// aView0.expInPlace()
assertEquals(F64Array.of(2.0, 3.0, 6.0, 7.0).reshape(2, 2), aView1)

Uncommenting the expInPlace line causes the test to fail, even though aView0 and aView1 don't intersect! This happens because viktor incorrectly assumes that aView0 is dense and applies expInPlace to elements 0..3 of the original a array.

We should fix this issue and add tests.

dievsky commented 5 years ago

And an inverted issue:

val a = DoubleArray(4) { it.toDouble() }.asF64Array().reshape(2, 2) // a 2x2 matrix
check(a.isDense) // passes
val aT = a.T // transposed matrix
check(aT.isDense) // fails

(Even though aT is a view of the same exact 4-element array.)

dievsky commented 5 years ago

We also have a wonderful reversed method which creates an array with negative strides. It could very well be dense, but it won't occupy the range [offset, offset + size), as all vector operations expect.

dievsky commented 5 years ago

It required a thorough rewrite (as well as killing transpose and reversed), but once issue-17 branch is merged, this issue will be history.

dievsky commented 5 years ago

Fixed by 1efb244c0f11dccf93ff3588709735485151a957.