-
Notifications
You must be signed in to change notification settings - Fork 89
Document all public symbols #647
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
Conversation
|
I probably need to copy the README blurb about sparse arrays somewhere |
| # Fields | ||
| - `static::Bool=false` |
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.
| # Fields | |
| - `static::Bool=false` |
This field will disappear soon anyway.
| # Fields | ||
| - `iPtr::JLVector{Ti, 1}`: indices of non-zero coefficients | ||
| - `nzVal::JLVector{Tv, 1}`: values of non-zero coefficients | ||
| - `len::Int`: size of the vector | ||
| - `nnz::Ti`: number of non-zero coefficients |
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.
I'm personally not a fan of unconditionally documenting all fields, especially when they are implicitly set by constructors and exposed through interface methods (nnzvals, length, etc). They aren't public API, but sadly there's no way to express that.
I'd be fine doing this for some other structures, so it's not a general rule though.
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.
I agree it's not ideal, but currently we're missing functions to access all of this information. For instance, SparseArrays has a rowvals function for CSC formats, but there is no equivalent colvals for CSR, and let's not even talk about BSR.
Based on my discussion with @kshyatt yesterday I understand that device-side arrays may not need to be documented as precisely, since their implementation will be here. However, host-side arrays need precise interfaces listing all their public fields, since people will want to write kernels on them?
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.
They aren't public API, but sadly there's no way to express that.
That's why I want to be careful about this PR, because in Julia 1.10 documented = public by default. In Julia 1.11 though we could add the public keyword!
|
How do I avoid running the full test suite for this PR? Is there a mechanism similar to CUDA's? |
|
I fixed the documentation, it should build now. Didn't want to force push a new commit message to avoid losing the thread, feel free to cut CI short. I guess the main question is what do you want to document, and what do you want to make public. Those don't necessarily have to be the same thing, but anything that is exported should probably be public, and thus documented. |
|
Superseded by #653 |
I did my best to add a docstring to all public symbols and include them in the documentation website. There were a few for which I didn't know what to say