Push to stable: matrix, blas, and lapack

[Brendan Tracey]

Short version: Everyone please take a look at the matrix API and voice any concerns / problems you see (includes mat64, cmat128, blas, lapack).

As you may know, we are pushing to have matrix and its dependencies reach 1.0 status. This is a big step and we hope this will encourage using matrix as a foundation for other packages and algorithms.

Over the past few months, matrix has undergone a lot of refinement and tightening of the API. The current API can be considered as "beta 1". The list of remaining issues can be seen here[0]. They all relate to the same fundamental question: what is the proper way to organize and interface between real and complex matrices?

I would like to request that everyone please take some time to review the matrix API, its subrepositories (mat64, cmat128), and its dependencies (blas, lapack). Everyone's participation is requested, it doesn't matter if you're a long time contributor or just observe the conversation on this list. We want to make sure we are happy with our decisions going forward. The stability promise (modeled after the Go 1.0 promise) can be found [2]. Please note that 1.0 status still allows for future additions, it just prevents changing what's there.

Thank you all for your help

[0] https://github.com/gonum/matrix/issues?q=is%3Aopen+is%3Aissue+milestone%3A%22API+stability%22
[1] godoc.org/github.com/gonum/matrix/
[2] https://gist.github.com/kortschak/e64da94971f3ae2c4046

[Vladimír Chalupecký]

I have done a first quick pass through mat64, I will need more time to go through the other packages and (at least try) to consider the real/complex matrix interfacing question.

So far mat64 API looks very reasonable and consistent to me. I noticed only a couple of things that called my attention:

- the name VectorSetter seems misleading to me and I would prefer a better name. Perhaps RowColSetter? Or maybe even better split it into two interfaces?

- RawColViewer returns *Vector while RawRowViewer returns []float64. It seems it would be natural for it to return []float64 too. On the other hand this interface is not used so we should remove it?

[Vladimír Chalupecký]

- the name VectorSetter seems misleading to me and I would prefer a better name. Perhaps RowColSetter? Or maybe even better split it into two interfaces?

Ah, of course, not only VectorSetter, also Vectorer.

[Brendan Tracey]

Should the current SetRow and SetCol really be "SetRawRow" and "SetRawCol" to match the Raw = []float64?

[Brendan Tracey]

Which begs the followon question if mat64.Col and mat64.Row should really be RawCol and RawRow.

[Brendan Tracey]

We currently have a "BandWidther" interface that as far as I know is completely unused. Maybe this should be removed until we know what the right method should be?

[Vladimír Chalupecký]

We currently have a "BandWidther" interface that as far as I know is completely unused. Maybe this should be removed until we know what the right method should be?

I was thinking the same.

Should the current SetRow and SetCol really be "SetRawRow" and "SetRawCol" to match the Raw = []float64?

Which begs the followon question if mat64.Col and mat64.Row should really be RawCol and RawRow.

Since the slices in these methods do not alias the backing row/column, adding a warning in the form of Raw is maybe not necessary but I can easily be convinced otherwise.

Re. Vectorer, VectorSetter: how about s/Vector/Slice/? Or split the two interfaces into 4 and call them Rower, RowSetter, Coler, ColSetter? 

[Dan Kortschak]

Vectorer method pairs are used together. If it's split, the code that
handles that case gets significantly more complex to describe, at least
in the case of Mul.

[Brendan Tracey]

Another idea before we fix API. Is it worth it to define

type Uplo bool

const(
    Upper = true
    Lower = false
)

so that the signature is
t := mat64.NewTriDense(n, matrix.Upper, nil)

It contains a lot more semantic content than 'true' does.

[Dan Kortschak]

On Wed, 2016-02-10 at 14:12 -0800, Brendan Tracey wrote:
> Another idea before we fix API. Is it worth it to define
>
> type Uplo bool
>
> const(
> Upper = true
> Lower = false
> )

Upper Uplo = true
Lower Uplo = false

> so that the signature is
> t := mat64.NewTriDense(n, matrix.Upper, nil)
>
> It contains a lot more semantic content than 'true' does.
>

That's not a huge change (essentially just code commenting in reality)
and would not break compatibility. I'm fine with it.

[Dan Kortschak]

On Thu, 2016-02-11 at 09:35 +1030, Dan Kortschak wrote:
> and would not break compatibility

In most cases.

[Brendan Tracey]

Okay, one more big crazy idea.

Maybe we should have one package for complex and real matrices. This would exist at gonum/matrix (or possibly gonum/mat -- it's going to be used a lot).

If we ever want to support single precision versions, those can exist at matrix/mat32

Downside: The complex signatures are uglier. I would propose that there be an appended "C", so it would be matrix.NewCDense, matrix.CMatrix, matrix.CDet.
- Reply: Compare cmat128.Dense and matrix.CDense. They both need that extra "C" moniker anyway, we're just shifting its location. If we switch to "mat", it's actually fewer characters as well; mat.CDense.
- Reply: In the signature of CMatrix, Dims() can be the same, and instead of T() we have H().  CAt() is by far the ugliest consequence, but I'm okay with that given the alternative

Downside: It breaks everything.
- Reply: We're going to be breaking everything anyway when we move to gonum.org. May as well follow in go's footsteps and transition into 1.0 with a bang

Upside: No conversion types
- The whole conv package is ugly and confusing. I've been thinking on and off about this, and I don't see a better way.
- As is, conv doesn't even solve the problem -- it is hard to write a function in mat64 that returns a complex matrix to then be used by cmat128. Circular imports really hurt here.
- The fact of the matter is, there are a fair number of functions that start with a matrix of real/complex and return the other. The most obvious is Eigen, which can return complex eigenvectors. In the other direction, there are operations like A^H * A  that produce a real symmetric matrix. If the two are the same package, the solution is easy
       CDense.Eigenvectors(eigen *Eigen)
       SymDense.CSymOuterK(cmat *CMatrix)
If they are in different packages, then there has to be something like a shell type for CDense that gets passed into conv to return a cmat128.Dense. Not pretty or intuitive. These kinds of runarounds suggest to me that it really should be one package

Upside: No numbers in the signatures
Much more minor than the previous point, but mat.Dense is much nicer than mat64.Dense

Upside: No common package
- There are already a large number of items that are shared in common between the two packages, including almost all of the package documentation.

Not a downside: Static typing
One may think that a common package would increase the coding errors, but for us static typing makes that a non-issue.

[Dan Kortschak]

I'm sort of concerned about the impact on the documentation (it become
very unwieldy). However, I think that's probably the limit of the
concerns.

We should try to get more input though.

> --
> You received this message because you are subscribed to the Google
> Groups "gonum-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to gonum-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Christopher Nielsen]

I haven't used gonum much, so feel free to take my thoughts with a
grain of salt.

Brendan,

I like the idea of combining the complex and real matrix packages into
one and shortening from matrix to mat; it seems more natural and
cleaner. Your argument about the conv package is a good one. As much
as possible, ugly packages that don't fully do what they're advertised
to do shouldn't be there.

Just my $0.02.

Chris

> --
> You received this message because you are subscribed to the Google Groups
> "gonum-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to gonum-dev+...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

--
Christopher Nielsen
"They who can give up essential liberty for temporary safety, deserve
neither liberty nor safety." --Benjamin Franklin
"The tree of liberty must be refreshed from time to time with the
blood of patriots & tyrants." --Thomas Jefferson

[Christopher Nielsen]

This idea seems to make code more readable and understandable. Having
more semantic content is better IMO.

[Vladimír Chalupecký]

I must say that after an initial hesitation I agree that this is indeed a good way forward.

I was hesitating because of the not-so-nice function names such as CDet but there are

not so many such functions and most importantly, often the appended C indicates not

only a different type (Det/CDet, Sum/CSum, Row/CRow) but also a slightly different

behavior (Dot/CDot). Some functions will not have their C equivalent (Max, Min).

Also, reduction in duplicate documentation is good (although some duplication will still persist).

Regarding CAt, it seems to me that it could be called just At even in CMatrix.

The two interfaces would be distinguished by T and H, and by the return type of At.

If this were possible, the Matrix and CMatrix interfaces would be really nice.

Or is there a reason to call it CAt that I don't see?

To put it another way, if we had started with a single package, would there be any substantial reason to split it into cmat and mat?

Lastly, a less serious upside: if it were called mat, I wouldn't need an extra keystrokes in the command-line completion to disambiguate between mat and mathext.

[Jonathan Lawlor]

The interfaces aren't the same anyway - float64 matrices have At(i, j int) float64, complex has At(i, j int) complex128

[Brendan Tracey]

To date we have an unofficial policy of not overwriting method names when the signatures are different (at least in mat64). This is why we have SymDense.MulSym and not SymDense.Mul. One core idea behind this is that it’s easier to follow the flow of types while reading. Dan can do the argument more justice than I can. 

That is one reason for it to be CAt. The other is the simple fact that it’s strange to have every complex function be preceded with a C, except the one. 

[Vladimír Chalupecký]

I imagined there would be a reason, calling it just At was too obvious. Originally, I thought that prepending C to package-level functions was out of necessity but prepending it also to methods is better. As you say, it will make it explicit that complex number are involved but also the method names will better correspond to interface names, e.g.,

type CRawRowViewer interface { CRawRowView(i int) []complex128 }

vs.

type CRawRowViewer interface { RawRowView(i int) []complex128 }