Simplify interfaces #8
Conversation
Fixed a few docs, and moved sort type to be next to the code that uses it.
I’ve removed the unused returns from functions with the pattern: func foo(dst []float, … ) []float where the returned slice was the same as the source slice, under the rationale that if people are using this code for tight loops without reallocation, then if the caller reallocates the return then they will add inadvertent inefficiency. Also, they might get the idea that the input is not modified. A function with no returned values is obviously one with only side effects. I also standardized the names of the input arguments - specifically, the ones that are modified in place are called “dst” Where functions had repeated types of inputs, I used the typical go syntax of func(x, y [type]) instead of func(x [type], y [type]) I also fixed capitalization for the package comment.
If either CumSum or CumProd had zero length inputs, they would have panicked. That behavior is not documented so I’ll assume this is a mistake.
Small change to avoid memory swapping.
Nearest was calculating the distance of the first element twice. Starting the loop on the second element avoids that.
There were a few missing edge cases, and a missing test for Fill. I tried to follow the style used in the existing tests, but it wasn’t consistent.
| @@ -18,10 +17,10 @@ import ( | |||
| // results stored in the first slice. | |||
| // For computational efficiency, it is assumed that all of | |||
| // the variadic arguments have the same length. If this is | |||
| // in doubt, EqLen can be used. | |||
| func Add(dst []float64, slices ...[]float64) []float64 { | |||
| // in doubt, EqualLengths can be used. | |||
There was a problem hiding this comment.
While we're in this edit, we should remove the variadic slice. Nothing else in this package does it. Also, if you don't mind, having an AddTo to compliment the other functions would be nice (it wasn't necessary with the variadic nature, but given that's going away). If you do mind, we can add it in a future PR
There was a problem hiding this comment.
Not a problem. While I'm at it, I might add an "Any" and "All" along the lines of Find.
|
I made lots of minor comments, but on the whole I like the changes. Thanks for making the cleanup pass. |
|
There's a lot goin on here - probably more than should be included in one PR. I agree with all the changes after a cursory view, with the exception of the changes to signatures. These changes should be preceeded by a discussion of the merits of the changes on gonum-dev. As Brendan has said there are reasons the []float64 values are returned. I'm happy for the changes to be made, but only if there is good justification for it - here is not the place to discuss that. |
|
Jon, before removing this entirely, submit a version with just the comment changes and the variable name changes (to dst, etc.). You can bring up a thread discussing the API changes. |
|
I'll work through the suggestions. I should have something committed tonight. Then I'll bring it up on the mailing list so we can focus on the bigger question of returning []float64 or not. @kortschak If you prefer, I can split out the interface changes from the other ones. The non-interface ones were so minor (in my mind) that I just wanted to fix them before I forgot about them. @btracey - I only closed it because it isn't ready to be pulled in its current form, I wasn't going to discard everything. |
|
As an aside what do you think of using a service like drone.io for CI testing? It only requires a build hook on go projects, then (if you are feeling sassy) an image in the readme. |
|
Yes, please take the significant API changes out of this PR, start a thread on gonum-dev detailing each of the changes with the justification for each (can be grouped if that makes sense). Then after that has been done, create a PR with the SGTM'd changes. Jeff uses travis, for gonum/graph and that seems good (we use it for cayley as well). |
Eliminated duplicated returns of the type ("Clean up func sigs")
func foo(dst []float64, ...) []float64 { ... }
where the returned slice was also dst. This makes the fact that those functions operate via side effect more explicit, and will prevent callers from redundantly assigning the result to a new slice.
I also cleared up a few edge cases in the code where zero length slices were provided to the functions.
The docs were cleaned up and variable names were made more uniform.
Testing has 100% coverage at this time.
What do you think of the changes?