Added out.clear for BIDMat DenseMat accum
Closed this issue · 3 comments
John, I added in out.clear lines for the DenseMat accum, i.e., what gets called when we do accum for CPU matrices.
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.
Good catch. accum was originally written before the output matrix was cached, and it didnt need clearing. But it does now.
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
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.
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.