Repository: gonum/gonum
Pull request #1611 (2021-04-14 20:46:03)

mat: add Tridiag to list_test
One of the tests fails due to floating point noise, so setting a draft status while we check how to best modify the test suite.

<!--
Checklist:

- API changes have been discussed
- code is goformated correctly (goimports)
- packages with generated code have had code generation run
- tests pass locally
- linked to relevant issues

Please make sure your commit message summary line and pull request
title match the Go convention; a one-line summary of the change,
prefixed by the primary affected package that should complete the
sentence, "This change modifies Gonum to _____."
-->


vladimir-ch: I've realized that it's enough to use `sameAnswerFloatApproxTol` instead of `sameAnswer` as input to `testOneInputFunc`. I've removed the Draft tag.
vladimir-ch: I'm wondering if tests like `TestTridiagTrace` or `TestTridiagNorm` could or should be removed. They're now redundant and extra test code to maintain.
codecov-commenter: # [Codecov](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum) Report
> Merging [#1611](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum) (8af0f73) into [master](https://codecov.io/gh/gonum/gonum/commit/ccc0da248a78266c0e8a38ac56e983d17c4ba15f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum) (ccc0da2) will **decrease** coverage by `0.05%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/gonum/gonum/pull/1611/graphs/tree.svg?width=650&height=150&src=pr&token=B7qHnqSPNL&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum)](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum)

```diff
@@            Coverage Diff             @@
##           master    #1611      +/-   ##
==========================================
- Coverage   73.04%   72.98%   -0.06%     
==========================================
  Files         498      492       -6     
  Lines       58813    58516     -297     
==========================================
- Hits        42961    42709     -252     
+ Misses      13424    13402      -22     
+ Partials     2428     2405      -23     
```


| [Impacted Files](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum) | Coverage Δ | |
|---|---|---|
| [mat/tridiag.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-bWF0L3RyaWRpYWcuZ28=) | `89.72% <100.00%> (+0.54%)` | :arrow_up: |
| [lapack/gonum/dlaqr1.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-bGFwYWNrL2dvbnVtL2RsYXFyMS5nbw==) | `40.62% <0.00%> (-15.63%)` | :arrow_down: |
| [optimize/listsearch.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-b3B0aW1pemUvbGlzdHNlYXJjaC5nbw==) | `72.22% <0.00%> (-1.86%)` | :arrow_down: |
| [mat/index\_bound\_checks.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-bWF0L2luZGV4X2JvdW5kX2NoZWNrcy5nbw==) | | |
| [graph/iterator/nodes\_map.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-Z3JhcGgvaXRlcmF0b3Ivbm9kZXNfbWFwLmdv) | | |
| [...sp/fourier/internal/fftpack/array\_bounds\_checks.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-ZHNwL2ZvdXJpZXIvaW50ZXJuYWwvZmZ0cGFjay9hcnJheV9ib3VuZHNfY2hlY2tzLmdv) | | |
| [graph/internal/set/same.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-Z3JhcGgvaW50ZXJuYWwvc2V0L3NhbWUuZ28=) | | |
| [mat/offset.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-bWF0L29mZnNldC5nbw==) | | |
| [graph/iterator/map.go](https://codecov.io/gh/gonum/gonum/pull/1611/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum#diff-Z3JhcGgvaXRlcmF0b3IvbWFwLmdv) | | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum). Last update [ccc0da2...8af0f73](https://codecov.io/gh/gonum/gonum/pull/1611?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=gonum).

vladimir-ch: I've removed TestTridiagTrace and now rely on list_test. It's a bit unclear and inconsistent in `mat` whether we want to test a functions on concrete types (Tridiag.Trace), an interface (mat.Tracer) or a package-level function (mat.Trace).

PTAL
kortschak: Not removing the Norm test?
kortschak: > It's a bit unclear and inconsistent in mat whether we want to test a functions on concrete types (Tridiag.Trace), an interface (mat.Tracer) or a package-level function (mat.Trace).

Maybe we should have an issue to decide on a policy for this?
romanwerpachowski: I would do whatever feels most natural in the context. Tests using a higher level API are more reusable, but may fail to test some particular error conditions which are implementation-specific. In my experience, in numerical code the veil of the OOP abstraction often needs to be "pierced" to achieve the best performance, or exploit the algorithm fully. Similar with testing numerical code. E.g. the tolerances may differ between different implementation of the same abstract interface.
kortschak: > I would do whatever feels most natural in the context. Tests using a higher level API are more reusable, but may fail to test some particular error conditions which are implementation-specific.

Yes. I think this is probably pretty much what we do. The list_test.go is there to give majority coverage with little work, but we also have specific tests for particular cases.

Patch:
diff --git a/mat/tridiag.go b/mat/tridiag.go
index afac4846..3537d34d 100644
--- a/mat/tridiag.go
+++ b/mat/tridiag.go
@@ -129,10 +129,10 @@ func (a *Tridiag) Reset() {
 	a.mat.DU = a.mat.DU[:0]
 }
 
-// Clone makes a copy of the input Tridiag into the receiver, overwriting the
-// previous value of the receiver. Clone does not place any restrictions on
-// receiver shape.
-func (a *Tridiag) Clone(from *Tridiag) {
+// CloneFromTridiag makes a copy of the input Tridiag into the receiver,
+// overwriting the previous value of the receiver. CloneFromTridiag does not
+// place any restrictions on receiver shape.
+func (a *Tridiag) CloneFromTridiag(from *Tridiag) {
 	n := from.mat.N
 	switch n {
 	case 0:
@@ -232,7 +232,7 @@ func (a *Tridiag) SolveTo(dst *Dense, trans bool, b Matrix) error {
 		dst.Copy(b)
 	}
 	var aCopy Tridiag
-	aCopy.Clone(a)
+	aCopy.CloneFromTridiag(a)
 	var ok bool
 	if trans {
 		ok = lapack64.Gtsv(blas.Trans, aCopy.mat, dst.mat)
@@ -263,7 +263,7 @@ func (a *Tridiag) SolveVecTo(dst *VecDense, trans bool, b Vector) error {
 		dst.CopyVec(b)
 	}
 	var aCopy Tridiag
-	aCopy.Clone(a)
+	aCopy.CloneFromTridiag(a)
 	var ok bool
 	if trans {
 		ok = lapack64.Gtsv(blas.Trans, aCopy.mat, dst.asGeneral())


Test patch:
diff --git a/mat/list_test.go b/mat/list_test.go
index 5f5687b3..c821bc54 100644
--- a/mat/list_test.go
+++ b/mat/list_test.go
@@ -238,7 +238,7 @@ func legalDims(a Matrix, m, n int) bool {
 		return true
 	case *SymDense, *TriDense, *basicSymmetric, *basicTriangular,
 		*SymBandDense, *basicSymBanded, *TriBandDense, *basicTriBanded,
-		*basicDiagonal, *DiagDense:
+		*basicDiagonal, *DiagDense, *Tridiag:
 		if m < 0 || n < 0 || m != n {
 			return false
 		}
@@ -320,6 +320,13 @@ func returnAs(a, t Matrix) Matrix {
 		case *basicDiagonal:
 			return asBasicDiagonal(mat)
 		}
+	case *Tridiag:
+		switch t.(type) {
+		default:
+			panic("bad type")
+		case *Tridiag:
+			return mat
+		}
 	}
 }
 
@@ -532,6 +539,17 @@ func makeRandOf(a Matrix, m, n int, src rand.Source) Matrix {
 			mat.SetDiag(i, rnd.Float64())
 		}
 		rMatrix = returnAs(mat, t)
+	case *Tridiag:
+		if m != n {
+			panic("bad size")
+		}
+		mat := NewTridiag(n, nil, nil, nil)
+		for i := 0; i < n; i++ {
+			for j := max(0, i-1); j <= min(i+1, n-1); j++ {
+				mat.SetBand(i, j, rnd.NormFloat64())
+			}
+		}
+		rMatrix = returnAs(mat, t)
 	}
 	if mr, mc := rMatrix.Dims(); mr != m || mc != n {
 		panic(fmt.Sprintf("makeRandOf for %T returns wrong size: %d×%d != %d×%d", a, m, n, mr, mc))
@@ -848,6 +866,10 @@ func makeCopyOf(a Matrix) Matrix {
 		}
 		copy(d.mat.Data, diag.mat.Data)
 		return returnAs(d, t)
+	case *Tridiag:
+		var m Tridiag
+		m.CloneFromTridiag(a.(*Tridiag))
+		return returnAs(&m, t)
 	}
 }
 
@@ -1058,6 +1080,10 @@ var testMatrices = []Matrix{
 	TransposeTri{&basicDiagonal{}},
 	TransposeBand{&basicDiagonal{}},
 	TransposeTriBand{&basicDiagonal{}},
+
+	&Tridiag{},
+	Transpose{&Tridiag{}},
+	TransposeBand{&Tridiag{}},
 }
 
 var sizes = []struct {
@@ -1139,7 +1165,7 @@ func testOneInputFunc(t *testing.T,
 				continue
 			}
 			if !sameAnswer(want, got) {
-				t.Errorf("Answer mismatch: %s", errStr)
+				t.Errorf("Answer mismatch: %s; got %v, want %v", errStr, got, want)
 			}
 		}
 	}
diff --git a/mat/matrix_test.go b/mat/matrix_test.go
index b9131fb6..cce4df6e 100644
--- a/mat/matrix_test.go
+++ b/mat/matrix_test.go
@@ -542,7 +542,7 @@ func TestTrace(t *testing.T) {
 	denseComparison := func(a *Dense) interface{} {
 		return Trace(a)
 	}
-	testOneInputFunc(t, "Trace", f, denseComparison, sameAnswerFloat, isAnyType, isSquare)
+	testOneInputFunc(t, "Trace", f, denseComparison, sameAnswerFloatApproxTol(1e-15), isAnyType, isSquare)
 }
 
 func TestTracer(t *testing.T) {
diff --git a/mat/tridiag_test.go b/mat/tridiag_test.go
index 2b385704..63ba57a9 100644
--- a/mat/tridiag_test.go
+++ b/mat/tridiag_test.go
@@ -325,18 +325,6 @@ func TestTridiagZero(t *testing.T) {
 	}
 }
 
-func TestTridiagTrace(t *testing.T) {
-	t.Parallel()
-	for _, n := range []int{1, 2, 3, 4, 7, 10} {
-		a, ref := newTestTridiag(n)
-		want := ref.Trace()
-		got := a.Trace()
-		if got != want {
-			t.Errorf("Case n=%d: unexpected trace: got %f, want %f", n, got, want)
-		}
-	}
-}
-
 func TestTridiagNorm(t *testing.T) {
 	t.Parallel()
 	for _, n := range []int{1, 2, 3, 4, 7, 10} {