-
-
Notifications
You must be signed in to change notification settings - Fork 117
[WIP] Module level documentation pass #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the module-level documentation for the cublas
module. The new documentation provides a usage guide, which is a great improvement. However, I've found a couple of inaccuracies in the guide regarding supported data types and how to call the API functions. My review includes suggestions to correct these points to avoid misleading users of the library.
//! | ||
//! 1. Instantiate a [CudaBlas] handle with [CudaBlas::new()] | ||
//! 2. Choose your operation: [Gemm], [Gemv], and [Asum] traits, which [CudaBlas] implements. | ||
//! 3. f16/bf16/f32/f64 are all supported at the trait level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement is incorrect. While the Gemm
trait has implementations for f16
, bf16
, f32
, and f64
, the Gemv
and Asum
traits only support f32
and f64
. This documentation is misleading to users of the library.
Please update the documentation to accurately reflect the supported types for each trait.
//! 3. Supported types: [Gemm] supports `f16`/`bf16`/`f32`/`f64`; [Gemv] & [Asum] support `f32`/`f64`.
//! 3. f16/bf16/f32/f64 are all supported at the trait level. | ||
//! 4. Instantiate your corresponding config: [GemmConfig], [StridedBatchedConfig], [GemvConfig], [AsumConfig] | ||
//! 5. Call using [CudaBlas::gemm()], [CudaBlas::gemv()], or [CudaBlas::asum()] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly misleading. The methods like gemm
are part of the Gemm
trait, not inherent methods of CudaBlas
. As a result, the rustdoc link [CudaBlas::gemm()]
will be broken.
It would be clearer to show an example of a method call on an instance, e.g., blas.gemm(...)
, and to hint that the trait needs to be in scope for the method to be available.
//! 5. Call the trait's method on the handle, e.g. `blas.gemm(...)`.
Missing quite a few things from module level documentation, so doing a pass over it.