Add some documentations for dense matrix. #105
Conversation
|
Please take a look at #26. This is the kind of documentation that needs to be added. This PR was stalled. |
|
@kortschak thanks for your reply, but I don't really understand what you mean. Do you mean Thanks. |
|
The documentation is needed, but the PR I mentioned is stalled. It would be nice if it was unstalled. |
|
Hi @lazywei , thanks for wanting to contribute. What @kortschak means is that #26 contains documentation additions more along the lines of the package style. If you could incorporate those changes into this PR that would be great. |
|
OK, so here is what I'm going to do: I'll try to merge (by hands) the documentation parts in #26, and try to modify the documentation style in my previous commit to match the guideline of this package. |
|
I would like to get agreement from @antoine-lizee for those to be incorporated. He has not been added to the A+C and #26 was not accepted for other reasons. If it were simple, I'd have gone through and done it my self. |
|
Sure, I have mentioned @antoine-lizee in that PR. I'll put this PR on hold then. |
|
Please go ahead! Thanks for asking. |
|
@antoine-lizee thanks! |
|
SGTM |
Please refer to these two Pull Requests gonum#26 and gonum#105 for more details of this commit. The original docs are written by @antoine-lizee (https://github.com/antoine-lizee), and this commit only modifies some content. Therefore, all credits should go to antoine.
|
I've pushed new commit, @kortschak please help me review them. |
|
Initial comments are that comment lines should around 80 character to ease reading and don't bother starting new lines unless you are starting a new paragraph in the godoc (or at ~80). |
| @@ -65,6 +65,11 @@ type Dense struct { | |||
| capRows, capCols int | |||
| } | |||
|
|
|||
| // NewDense creates a new matrix of type *Dense with dimensions r and c. If the mat argument is nil, | |||
| // a new data slice is allocated. | |||
| // Data slice is of Row-major order, i.e., the (i, j) element in matrix should be at (i*c + j)-th | |||
There was a problem hiding this comment.
The data must be arranged in row-major order, i.e. the (i*c + j)-th element in mat is the {i, j}-th element in the matrix.
| // Mul takes the matrix product of a and b, placing the result in the receiver. | ||
| // Note that the arguments a or b should be either Matrix or *Dense. | ||
| // Therfore, if a or b is of type Dense, you'll need to pass them by address. | ||
| // For example: m.Mul(a, &b) when a is *Dense and b is Dense. |
There was a problem hiding this comment.
Remove the lines after the first. This is better covered by an example.
There was a problem hiding this comment.
As mentioned, where should we put this into an example?
There was a problem hiding this comment.
It doesn't need to be here. It's not specific to Mul.
|
When I say "Example", I mean in the testing sense. See the Example section in the overview of http://golang.org/pkg/testing/ |
| // mat argument is nil, a new data slice is allocated. | ||
| // The data must be arranged in row-major order, i.e. the (i*c + j)-th element | ||
| // in mat is the {i, j}-th element in the matrix. | ||
| // See Reset for how to use zero-sized matrix. |
There was a problem hiding this comment.
godoc needs a blank comment line to start a new paragraph.
|
SGTM |
|
Sorry that I'm a little buzy this weeks. I'll finish the rest work in the following 2~3 days. |
|
Get the diff between your PR state and the state at gonum/matrix in the docs branch and apply those, then I'm happy, though @btracey may have comments on those.
I picked up a number of typos in the current state of your PR when I edited that, so they need to be fixed. My PR also adds comments to newly added methods. We'll add to the other things later.
|
Please refer to these two Pull Requests gonum#26 and gonum#105 for more details of this commit. The original docs are written by @antoine-lizee (https://github.com/antoine-lizee), and this commit only modifies some content. Therefore, all credits should go to antoine.
|
@kortschak I've finished the rebase. I'm not sure how to add the example test and the doc for capRow/capCol, so I'll leave it to you guys, is that OK? Thanks. |
|
I think this is just easier if I merge #113, |
Current documentation is not clear unless users look into the source code. So I decide to add some documentations to several common methods.
Please let me know if there is any concern.
Thanks.