search

BIDData / BIDMat

A CPU and GPU-accelerated matrix library for data mining
BSD 3-Clause "New" or "Revised" License
265 stars 73 forks source link

Added out.clear for BIDMat DenseMat accum #34

Closed DanielTakeshi closed 9 years ago

DanielTakeshi commented 9 years ago

John, I added in out.clear lines for the DenseMat accum, i.e., what gets called when we do accum for CPU matrices.

https://github.com/BIDData/BIDMat/commit/99e99233c500440b22ae23adacc80067895f715d

The issue was that originally, my accum calls would store the results from the previous accum call in out which resulted in cumulative sums being added to the previous output, not a clear matrix of zeros. The GPU versions of accum have out.clear in them (check GMat.scala) so I'm assuming that the lack of out.clear in the CPU versions is just an oversight. I didn't catch this in testing but it came up in the BayesNet.scala tests with matrix caching on.

John, assuming this doesn't break anything, feel free to close this.

jcanny commented 9 years ago

Good catch. accum was originally written before the output matrix was cached, and it didnt need clearing. But it does now.

jcanny commented 9 years ago

Daniel, I inlined some code suggestions for BayesNet. I realized that taking single multinomial samples (which are actually categorical variables) is so simple there is really no need to write a function for it. It uses a new function (cummaxByKey) which I added to BIDMat.

The new code is complete and should run on CPU or GPU.

-John

DanielTakeshi commented 9 years ago

I found another missing out.clear problem. It's in the SparseMat.scala class, for the full method. Without adding in the out.clear as in the commit below, my repeated calls of full(sdata), where sdata is a sparse data matrix, would have previous data "filled in" at inappropriate locations.

https://github.com/BIDData/BIDMat/commit/701ffbb136b4a97a0f12d67993122c0d48d0ebf3

Edit: to be more precise, when calling the full method, we only iterate through the known elements of the sparse matrix, which leaves the previously known values untouched. The GPU version avoids this problem with an out.clear.