Mulgen by jonlawlor · Pull Request #61 · gonum/matrix

jonlawlor · 2014-12-29T03:24:30Z

as per #54

coveralls · 2014-12-29T03:28:52Z

Coverage decreased (-2.07%) when pulling 89baed7 on mulgen into 3ec7a06 on master.

jonlawlor · 2014-12-29T13:31:28Z

So, one question is - should MulGen use bool to represent transposes, or should it use blas.Transpose types, which can represent conjugate transpose as well?

btracey · 2014-12-29T13:35:36Z

mat64/dense_arithmetic.go

Is MulGen the right name, or should it be GenMul?

We should have some function comment here.

I think we can come up with something better than at and bt.

If it wasn't clear, I'm not sure about the name. MulGen might be better, because Mul is the obvious name and MulGen will be next in the list.

When we talked about it in #54, @kortschak named it, and I think having it in the order MulGen is better because then in godoc it will show up next to Mul and MulElem.

If you mean that the interface could be GenMuler instead of MulGener, that wouldn't be consistent with the typical naming convention for single method interfaces.

Instead of at and bt I was thinking of switching it to tA and tB, to mirror the inputs to blas.Dgemm, and change the type from bool to blas.Transpose so that if we get complex matrices the interface will still work. Alternatively, we could call it transA and transB to mirror netlib's quick reference at http://www.netlib.org/blas/

Or we could do something else, any suggestions?

I personally prefer aTrans and bTrans to either tA or transA

Also, many of the Dense methods don't have docs, apparently in favor of having them with the interfaces defined in matrix.go. I don't know if that's right or wrong, I've seen it both ways. On the one hand, it reduces the number of places we have to check comments for consistency, but on on the other golint gets mad.

jonlawlor · 2014-12-30T00:18:32Z

I'll be making those changes, and I also went ahead with a benchmark of CovarianceMatrix using MulGen to avoid a call to TCopy and it runs ~35% faster on my crappy macbook using Accelerate.

benchmark                                         old ns/op      new ns/op      delta
BenchmarkCovarianceMatrixSmallxSmall              17928          13997          -21.93%
BenchmarkCovarianceMatrixSmallxMedium             20735644       21302799       +2.74%
BenchmarkCovarianceMatrixMediumxSmall             399850         252919         -36.75%
BenchmarkCovarianceMatrixMediumxMedium            320923951      173250045      -46.02%
BenchmarkCovarianceMatrixLargexSmall              56628605       39087916       -30.97%
BenchmarkCovarianceMatrixHugexSmall               6571752633     4337103457     -34.00%
BenchmarkCovarianceMatrixSmallxSmallInPlace       11989          8847           -26.21%
BenchmarkCovarianceMatrixSmallxMediumInPlace      15443508       15929012       +3.14%
BenchmarkCovarianceMatrixMediumxSmallInPlace      370849         238972         -35.56%
BenchmarkCovarianceMatrixMediumxMediumInPlace     306710031      166414344      -45.74%
BenchmarkCovarianceMatrixLargexSmallInPlace       55798314       38903944       -30.28%
BenchmarkCovarianceMatrixHugexSmallInPlace        5709754453     3982521912     -30.25%

kortschak · 2014-12-30T00:20:46Z

mat64/dense_arithmetic.go

Delete line.

The inputs should have been transposed. Trying to use this in the stats package panic’ed, and the BLAS docs indicate that the dims should be *after* the op.

coveralls · 2014-12-30T00:49:23Z

Coverage decreased (-2.11%) when pulling a3575db on mulgen into 3ec7a06 on master.

jonlawlor · 2014-12-30T01:00:39Z

I'll rework them a bit to make it clearer.

jonlawlor · 2014-12-30T02:44:37Z

I still have to write tests for the Vectorer and Matrix code paths, fix whatever's wrong, and we have to resolve if methods in Dense should all have godoc comments.

coveralls · 2014-12-30T02:47:40Z

Coverage decreased (-2.23%) when pulling 75c1444 on mulgen into 3ec7a06 on master.

btracey · 2014-12-30T02:52:23Z

Just throwing this out there -- the actual BLAS function is C = beta * C + alpha * A * B. The beta part is easily solved with Scale (though at some performance penalty), but the "+=" part I think is only solvable with a temporary allocation. We should probably figure out if we want a full mat64 wrapper to the BLAS function, rather than the partial wrapper that this is, and if so, what we want to call it. It seems like if we want the full wrapper, GenMul is the wrong name, and so there is no problem here, but I just wanted to mention it.

jonlawlor · 2014-12-30T03:15:10Z

Then maybe MulTrans would be more appropriate?

coveralls · 2014-12-30T04:50:07Z

Coverage increased (+0.74%) when pulling 586c8a9 on mulgen into 3ec7a06 on master.

kortschak · 2014-12-30T09:44:55Z

mat64/dense_test.go

Just type denseAt Dense will do this for you - also below (just type convert to get the *Dense method when you want it implemented). I thought we had already fabricated types like this in the tests though - yes: https://github.com/gonum/matrix/blob/master/mat64/mul_test.go#L225

kortschak · 2014-12-30T10:27:22Z

I'm good with MulTrans if we are going to add a MulGen later.

btracey · 2014-12-30T13:53:28Z

Would we call the function MulGen though? It has a "+=" component which would be different that the other Muls, and different than basically all of the other operations where the receiver is overwritten.

jonlawlor · 2014-12-30T14:01:19Z

It would kind of be a matrix version of a fused multiply add. Maybe call that one FusedMulAdd and name this MulTrans?

These were already implemented in mul_test.go

jonlawlor · 2014-12-31T02:40:47Z

I've made the changes you've requested. Is there anything else?

coveralls · 2014-12-31T02:41:36Z

Coverage increased (+0.74%) when pulling b1f0785 on mulgen into 3ec7a06 on master.

kortschak · 2014-12-31T04:18:07Z

mat64/dense_arithmetic.go

Should we not test that it's not blas.ConjTrans and panic if it is? Also below. If we're using a blas implementation that should catch this, but it won't a or b are the slow case.

I agree with @kortschak . We should panic if it's just some random number.

Do we want to have our own constants in mat64 (aka mat64.Transpose)? It seems desirable that users never have to interface with blas beyond registering. It would also avoid the problem where blas actually accepts ConjTrans as a valid input for DXxx cases. Instead, we define

const (
NoTrans = blas.NoTrans
Trans = blas.Trans
)

and now it's logical to panic on ConjTrans because ConjTrans doesn't exist in mat64

I made it perform a transpose when given a ConjTrans because DGEMM's docs have TRANSA = 'C' or 'c', op( A ) = A'.. The conjugate transpose is defined as the element-wise complex conjugate of the transpose, so in the case of real numbers, it is the same as the transpose, so the behavior is implemented as defined. It sounds like panicing would be a special case where none is needed.

I don't have an issue with putting constants into mat64 to make things easier on users.

but it still should be

if bTrans == mat64.Trans{
br, bc = bc, br
}else if bTrans != mat64.NoTrans{
panic("mat64: invalid Transpose value")
}

In other words, if I pass -1 into the function it shouldn't act as Transpose.

One of the ways we could get a real matrix from a complex matrix would be to do

m := NewDense(0,0,nil) m.MulTrans(A, mat.NoTrans, A, mat.TransConj)

edit: where A is a possibly complex matrix.
edit2: I don't know why I thought that would always be true, but it isn't. Sorry, it is late.

The constants are definitely better if we ever plan on using complex matrices. Otherwise the MulGen sig for a ComplexDense (or whatever) would not be able to express one of the transpose operations.

But A is not a possibly complex matrix. mat64.Matrix has At which returns a float64. A complex matrix by definition would have to return complex128 from At. In mat128, it will probably need constants. One of the penalties with Go's simplicity is that it will be impossible to mesh the packages fully.

I hadn't considered that - you're right, Matrix can't do complex.

Alright, I'm fine with either bool (which is what I initially implemented, then scared myself into switching to the flags) or using the flags and panicing. I'd lean towards the bools, because that's one fewer panic that can occur, and callers won't have to look up what to put into it.

We think the constants are better than a boolean even though it can only take two values here?

I completely missed that. It should be bool.

kortschak · 2014-12-31T04:22:55Z

LGTM after discussion of above, but also wait for @btracey.

btracey · 2014-12-31T06:06:38Z

mat64/dense_arithmetic.go

On all of these, you can have a

dataTmp := w.mat.Data[r_w.mat.Stride: r_w.mat.Stride + bc]
and then have dataTmp[c]. Probably doesn't save that much given the rest of the inefficiencies, but may as well.

The trouble is that would require a type assert to access the mat field, and we don't currently have a type that implements Vectorer but doesn't implement RawMatrixer.

You don't need to assert to get the mat field; w is a Dense.

Ah, yes you're right.

coveralls · 2015-01-01T04:13:18Z

Coverage increased (+0.83%) when pulling 2bce680 on mulgen into 3ec7a06 on master.

jonlawlor · 2015-01-01T04:29:55Z

I've changed the code to use booleans, but I don't see how I can implement @btracey's suggestion. Is it good to go without it?

kortschak · 2015-01-01T05:07:26Z

LGTM but again wait for @btracey.

By taking a slice once, we can avoid doing some extra math on each loop.

jonlawlor · 2015-01-01T05:44:34Z

OK, I've implemented that as well. I haven't benchmarked it though.

coveralls · 2015-01-01T05:48:14Z

Coverage increased (+0.9%) when pulling 256229d on mulgen into 3ec7a06 on master.

btracey · 2015-01-01T07:45:15Z

LGTM.

Mulgen

jonlawlor added 4 commits December 28, 2014 17:15

Add MulGener interface

a6886fa

initial impl of MulGen

1fdbb8e

add test for MulGen

aa35059

add todo for dsyrk

89baed7

btracey reviewed Dec 29, 2014
View reviewed changes

kortschak reviewed Dec 30, 2014
View reviewed changes

mat64/dense_arithmetic.go Outdated

Copy link
Copy Markdown

Member

kortschak Dec 30, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete line.

fix dims sent to Dgemm

a3575db

The inputs should have been transposed. Trying to use this in the stats package panic’ed, and the BLAS docs indicate that the dims should be *after* the op.

jonlawlor added 2 commits December 29, 2014 21:37

change MulGener interface to take blas.Transpose

18d4090

change impl and tests to use blas.Transpose

75c1444

add tests for vectored and matrix code paths

586c8a9

kortschak reviewed Dec 30, 2014
View reviewed changes

jonlawlor added 2 commits December 30, 2014 21:35

remove redundant type conversions

984fc3d

These were already implemented in mul_test.go

s/MulGen/MulTrans/g

b1f0785

kortschak reviewed Dec 31, 2014
View reviewed changes

btracey reviewed Dec 31, 2014
View reviewed changes

jonlawlor added 2 commits December 31, 2014 23:08

change MulTrans to use bool for transpose flag

e01fb9c

Adjust test and impl to use bool for Transpose flag

2bce680

make output row slice indexing more efficient

256229d

By taking a slice once, we can avoid doing some extra math on each loop.

jonlawlor added a commit that referenced this pull request Jan 1, 2015

Merge pull request #61 from gonum/mulgen

8b25631

Mulgen

jonlawlor merged commit 8b25631 into master Jan 1, 2015

jonlawlor mentioned this pull request Jan 1, 2015

Transpose Multiply Operation #54

Closed

jonlawlor deleted the mulgen branch January 1, 2015 23:36

Conversation

jonlawlor commented Dec 29, 2014

Uh oh!

coveralls commented Dec 29, 2014

Uh oh!

jonlawlor commented Dec 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonlawlor commented Dec 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 30, 2014

Uh oh!

jonlawlor commented Dec 30, 2014

Uh oh!

jonlawlor commented Dec 30, 2014

Uh oh!

coveralls commented Dec 30, 2014

Uh oh!

btracey commented Dec 30, 2014

Uh oh!

jonlawlor commented Dec 30, 2014

Uh oh!

coveralls commented Dec 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kortschak commented Dec 30, 2014

Uh oh!

btracey commented Dec 30, 2014

Uh oh!

jonlawlor commented Dec 30, 2014

Uh oh!

jonlawlor commented Dec 31, 2014

Uh oh!

coveralls commented Dec 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kortschak commented Dec 31, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment