gonum / blas

A BLAS implementation for Go [DEPRECATED]
172 stars 16 forks source link

cgo: add docs for generated functions #189

Closed kortschak closed 7 years ago

kortschak commented 8 years ago

This PR should have more full agreement people-wise; it does not add docs to all functions - only those that are generated and have cognate Go implementations with docs. It is possible to add docs to the complex functions by adding source files for the complex implementations in native with a non-building build tag. The handwritten functions here again will need changes (templating) to bring them in.

@btracey @vladimir-ch Please take a look.

btracey commented 8 years ago

Sorry, I don't understand what the comments like "// cblas.h:587:6 void cblas_zher2k ..." do.

What would you like reviewed? The comments themselves? The idea of generating comments automatically?

kortschak commented 8 years ago

The origin notation just tells where the signature was taken from. We don't have to do that - it was easy and might be helpful.

The concept for review is whether we want to add incomplete documentation and documentation that was written for the native package; some may be incorrect because we have documented for the behaviour of native where the behaviour is not specified by BLAS.

vladimir-ch commented 7 years ago

Which are the routines where the behavior differs? Could they be put on an exclude list?

kortschak commented 7 years ago

I know there are cases where BLAS behaviour is not defined for NaN and we specify a behaviour. That would be misleading. @btracey would have the details - I don't recall.

btracey commented 7 years ago

@kortschak @vladimir-ch

The BLAS operations aren't defined if the values are NaN. The one that I know has mismatches is Idamax. The implementations could also plausibly vary on C = beta * C + alpha*A*B. If alpha is 0, implementations usually skip the A*B part, even though NaN * 0 is zero.

btracey commented 7 years ago

(including ours)

vladimir-ch commented 7 years ago

We could copy all the function comments and say in the docs in the blas package that the behaviour of various implementations in presence of NaNs may differ.

btracey commented 7 years ago

I took a quick look at the documentation for all the D**** functions. I believe they are all accurate for all BLAS implementations, with a few exceptions. One is Drotg, and the note there (but I believe the . I believe the second is Idamax, where some implementations don't return the first such instance. The last is differences with NaN values, but this should be a high level documentation, since it's true for all BLAS functions.

Otherwise I'm in favor of autogenerated documentation, even if it's partial.

I might pre-pend "call at" to the reference lines: call at cblas.h:24:8. float cblas_sdsdot ...

kortschak commented 7 years ago

It would be "declared at" rather than "call at" since this is the header declaration, but that would be OK.

kortschak commented 7 years ago

Although, the notation that is currently used should already be generally understood to be a link to a source file declaration.

btracey commented 7 years ago

LGTM