Skip to content
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

docs: Add stubs for some sparse APIs #1768

Merged
merged 13 commits into from
Apr 25, 2023

Conversation

e10harvey
Copy link
Contributor

@e10harvey e10harvey commented Mar 31, 2023

@jgfouca: Please fill in the comment content for par_ilut
@jennloe: Please fill in the comment content for gmres
@brian-kelley: Please fill in the comment content for gauss_seidel and block_gauss_seidel
@lucbv: Please fill in the comment content for spgemm and block_spgemm

Feel free to open PRs against more_sparse_docs or just use the suggested changes feature.

Using AT: WIP since there's no need to run more than github-DOCS and github-FORMAT on these changes.

docs/developer/contrib.rst Outdated Show resolved Hide resolved
@@ -44,6 +44,17 @@ namespace Experimental {
std::is_same<typename std::remove_const<A>::type, \
typename std::remove_const<B>::type>::value

/// @brief
Copy link
Contributor

@jgfouca jgfouca Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @e10harvey! You are working off a fork, so I can't make changes directly to the branch, but here is what I would put for the brief:

Performs the symbolic phase of the par_ilut algorithm (described in the header). The sparsity pattern of A will be analyzed and L_rowmap and U_rowmap will be populated with the L (lower triangular) and U (upper triagular) non-zero counts respectively. Having a separate symbolic phase allows for reuse when dealing with multiple matrices with the same sparsity pattern. This routine will set some values on handle for symbolic info (row count, nnz counts).

/// @tparam KernelHandle Template for the KernelHandle type
/// @tparam ARowMapType Template for A_rowmap type
/// @tparam AEntriesType Template for A_entries type
/// @tparam LRowMapType Template for L_rowmap type
/// @tparam URowMapType Template for U_rowmap type
/// @param handle The kernel handle. It is expected that create_par_ilut_handle has been called on it
/// @param A_rowmap The row map (row nnz offsets) for the A CSR (Input)
/// @param A_entries The entries (column ids) for the A CSR (Input)
/// @param L_rowmap The row map for the L CSR, should already be sized correctly (numRows+1) (Output)
/// @param U_rowmap The row map for the U CSR, should already be sized correctly (numRows+1) (Output)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgfouca I don't know the details of the underlying algorithm. Would it be accurate to say that the symbolic phase determines the graphs (sparsity patterns) of the $L$ and $U$ factors?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhux2 , yes that's right. I think the idea is that there may be a use case where the user has multiple matrices with different values but the same sparsity pattern, so you can reuse the symbolic phase for all of them.

@@ -165,6 +176,28 @@ void par_ilut_symbolic(KernelHandle* handle, ARowMapType& A_rowmap,

} // par_ilut_symbolic

/// @brief
Copy link
Contributor

@jgfouca jgfouca Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here's the dox for this one:

Performs the numeric phase (for specific CSRs, not reusable) of the par_ilut algorithm (described in the header). It is expected that par_ilut_symbolic has already been called for the provided KernelHandle.

/// @tparam KernelHandle Template for the handle type
/// @tparam ARowMapType Template for the A_rowmap type
/// @tparam AEntriesType Template for the A_entries type
/// @tparam AValuesType Template for the A_values type
/// @tparam LRowMapType Template for the L_rowmap type
/// @tparam LEntriesType Template for the L_entries type
/// @tparam LValuesType Template for the L_values type
/// @tparam URowMapType Template for the U_rowmap type
/// @tparam UEntriesType Template for the U_entries type
/// @tparam UValuesType Template for the U_values type
/// @param handle The kernel handle. It is expected that create_par_ilut_handle has been called on it
/// @param A_rowmap The row map (row nnz offsets) for the A CSR (Input)
/// @param A_entries The entries (column ids) for the A CSR (Input)
/// @param A_values The values (non-zero matrix values) for the A CSR (Input)
/// @param L_rowmap The row map (row nnz offsets) for the L CSR (Input/Output)
/// @param L_entries The entries (column ids) for the L CSR (Output)
/// @param L_values The values (non-zero matrix values) for the L CSR (Output)
/// @param U_rowmap The row map (row nnz offsets) for the U CSR (Input/Output)
/// @param U_entries The entries (column ids) for the U CSR (Output)
/// @param U_values The values (non-zero matrix values) for the U CSR (Output)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am leaving the deterministic arg undocumented since it is going away as soon as my latest par_ilut PR goes in.

@jgfouca
Copy link
Contributor

jgfouca commented Mar 31, 2023

@e10harvey , where does the doxygen for the various handle classes end up? I'm having trouble finding it on the RTD page.

@e10harvey
Copy link
Contributor Author

e10harvey commented Apr 4, 2023

@e10harvey , where does the doxygen for the various handle classes end up? I'm having trouble finding it on the RTD page.

@jgfouca: The source is being parsed already but not yet included via the rst stubs we're adding here. If you add the doxygen-style comment to your handle class(es), you can tell sphinx to pull that in via this in the par_ilut section of sparse.rst:

.. doxygenclass::    KokkosSparse::PAR_ILUTHandle
    :members:

NOTE: If you were to references KokkosSparse:PAR_ILUTHandle somewhere else in our docs, sphinx will auto-generate links to your handle API docs.

@jgfouca
Copy link
Contributor

jgfouca commented Apr 4, 2023

Thanks @e10harvey , I have a brief doxy comment above the handle class and some decent doxys on the members which I think are the most important things to document.

e10harvey and others added 3 commits April 4, 2023 11:06
@e10harvey
Copy link
Contributor Author

@jgfouca, @jhux2: Any more changes for par_ilut?

@lucbv, @jennloe, @brian-kelley: I am thinking of just merging this with the par_ilut changes. You can go back and write the docs in follow-on PRs. Although, preferably this would already be done. My hope is to make things easier for everyone by adopting a process of writing and reviewing the documentation during the development process rather than having extra work in a follow-on PR / wiki edit change like this. In order for this to succeed, we have to hold each other accountable for updating the doxygen comments and .rst files during code reviews.

@jgfouca
Copy link
Contributor

jgfouca commented Apr 10, 2023

@e10harvey , looks good to me!

@brian-kelley
Copy link
Contributor

@e10harvey I added the Gauss-Seidel comments in a PR against your branch: e10harvey#15

Add doxygen for user-facing Gauss-Seidel functions
@e10harvey
Copy link
Contributor Author

@lucbv, @jennloe: Do you want to open a follow-on PR or add the spgemm and gmres docs to this one?

@e10harvey e10harvey merged commit cb9fc79 into kokkos:develop Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants