gonum / matrix

Matrix packages for the Go language [DEPRECATED]
446 stars 53 forks source link

ma64: Cholesky and LU need a Reset method #379

Closed vladimir-ch closed 8 years ago

vladimir-ch commented 8 years ago

Both methods panic when the size of the receiver and argument mismatch:

    if orig != c {
        if c.isZero() {
            c.chol = NewTriDense(n, matrix.Upper, nil)
        } else if c.chol.mat.N != n {
            panic(matrix.ErrShape)
        }
        c.chol.Copy(orig.chol)
    }

Wouldn't reusing it be more natural and useful?:

    if orig != c {
        if c.isZero() {
            c.chol = NewTriDense(n, matrix.Upper, nil)
        } else {
            c.chol = NewTriDense(n, matrix.Upper, use(c.chol.mat.Data, n*n))
        }
        c.chol.Copy(orig.chol)
    }

Otherwise the only way to reuse a factorization is through Factorize. The panic error ErrShape does not feel right either.

vladimir-ch commented 8 years ago

Perhaps I should have made it a question: Should factorizations be dimensionally restricted like matrices? Maybe they should but then they need a Reset method.

btracey commented 8 years ago

I agree with your last sentence, they should but with a reset method. @kortschak ?

kortschak commented 8 years ago

That would be consistent with the rest of the API. Yes.

vladimir-ch commented 8 years ago

Ok, I changed the title of this issue. Now it seems obvious.

Another couple of questions comes to my mind:

I'm not sure if this issue is the best place for these questions but at least I write them here before they escape my mind. Also, it seems to me that this issue and the above questions fall under the API stability effort.

kortschak commented 8 years ago
  1. I think this is OK as is.
  2. Probably not, though I'm not adamant about this. A workspace could be used to make the operations atomic.
  3. Making the operation atomic would solve this.
vladimir-ch commented 8 years ago

Closed by #378 and #382