Simplify interfaces retry #12
Conversation
As detailed in https://groups.google.com/forum/#!topic/gonum-dev/kOSNjAYGpPc Changed sigs for most functions which mutated inputs to no longer return values. Also made Add and AddTo align with other arithmetic ops.
Some examples and tests were no longer sensible, and were removed. The other ones were changed to use the new Add and AddTo functions.
Add a test for new function AddTo. It was covered except for the panics by the example code. This brings coverage to 100%
These changes didn’t result in a performance improvement. Rolling them back.
Two of the public doc comments were missing periods at the end of sentences. Fixed.
|
Remove now unnecessary returns statements and get rid of superfluous named returns (not your blame, but since you are there). |
|
Will do. |
There was no need for many of these return statements, and many of the return variable meanings were quite obvious. In particular, the LogSumExp function wasn’t actually returning the same variable. The rest of the changes are mostly cosmetic.
|
Changed. How does it look now @kortschak ? |
| @@ -517,9 +501,8 @@ func MulTo(dst, s, t []float64) []float64 { | |||
| // Panics if len(s) == 0. | |||
| func Nearest(s []float64, v float64) (ind int) { | |||
There was a problem hiding this comment.
Remove ind named return (unless @btracey violently disagrees), then LGTM
|
I'm confused. Did we decide that we were removing all of the returns, or did we decide to adopt the rule that there is a return iff calling the function multiple times will give the same result. |
|
@btracey So then, I'll add the returns back in to the "FooTo" functions. @kortschak - I can remove that named return. I was leaving it there to show that it was the index that was returned, not the value. Something has been bothering me about the Nearest function though. Specifically, there seems to be the possibility of overflow in the function, and an issue with aliasing, neither of which is handled. Do we want to make sure to (for example) return the right index when calling Nearest([]float64{1, 2, 3}, math.MaxFloat64)Which will return the first value because the difference between them is beyond the resolution of Float64 when compared with It isn't possible to handle all of the cases, but some of them can be handled by using several comparisons in place of subtraction, except where the compared value I'll set up a branch with the test, a possible fix, and benchmarks for the current impl and fix impl. |
|
@btracey sorry, the latter. Thank you for being careful. @jonlawlor, Nearest returns an int, so it can't return the value and the comment is only one line above. I understand the concern (I was considering it), but I don't think it should be a problem. |
Functions which always result in the same dot slice should return slices, as agreed during discussion in gonum-dev. I also added tests to make sure that the returned slices were the same as the mutated slices. Even though the coverage was reporting 100%, the tests were not covering 100% of the returned values.
what it says on the tin
|
@btracey - I added the returns back into the FooTo functions, and added tests for the returned values. Should be good to go. |
|
Also, thanks for bearing with me on these changes, I'll try to be more careful (and less noisy) in the future. |
|
For the bug I mentioned see https://github.com/gonum/floats/tree/bug-nearest, specifically 0872e42 |
| @@ -62,22 +65,13 @@ func AddScaled(dst []float64, alpha float64, s []float64) { | |||
| // It panics if the lengths of dst, y, and s are not equal. | |||
| // | |||
| // At the return of the function, dst[i] = y[i] + alpha * s[i] | |||
| func AddScaledTo(dst, y []float64, alpha float64, s []float64) []float64 { | |||
| func AddScaledTo(dst, y []float64, alpha float64, s []float64) { | |||
|
LGTM with the last couple of slices returned that I marked. |
Added missing returned slices for AddScaledTo, CumSum, and CumProd. Also added tests for the returned slices.
|
OK, changes implemented. I'm going to merge now. |
Simplify interfaces, as outlined in https://groups.google.com/forum/#!topic/gonum-dev/kOSNjAYGpPc
Changes as outlined in https://groups.google.com/forum/#!topic/gonum-dev/kOSNjAYGpPc
It looks like there is consensus to make them?