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

Fixing broken doc functions and improving coverage #1030

Merged
merged 6 commits into from
Nov 24, 2022

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Nov 18, 2022

No description provided.

@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 18, 2022
@cjnolet cjnolet requested a review from a team as a code owner November 18, 2022 00:42
@github-actions github-actions bot added the cpp label Nov 18, 2022
@cjnolet cjnolet requested a review from a team as a code owner November 18, 2022 20:48
@cjnolet
Copy link
Member Author

cjnolet commented Nov 21, 2022

rerun tests

1 similar comment
@cjnolet
Copy link
Member Author

cjnolet commented Nov 22, 2022

rerun tests

Copy link
Member

@benfred benfred left a comment

Choose a reason for hiding this comment

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

looks good! I like that this expands the docs coverage - and we're now including all the pylibraft modules in the sphinx docs.

Left a couple minor comments below,

cpp/include/raft/core/mdspan.hpp Show resolved Hide resolved
cpp/include/raft/random/rng.cuh Outdated Show resolved Hide resolved
docs/source/cpp_api/mdspan.rst Show resolved Hide resolved
Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Hi Corey, nice to see these updates! I have some suggestions below, but most of these can be implemented in a follow up PR. Overall the current PR looks good to me, pre-approving.

namespace raft::random {

/**
* @brief Matrix decomposition method for `compute_multi_variable_gaussian` to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this docstring supposed to appear in the API docs? I do not see it there.

-------------------
###################

Header: `raft/spectral/partition.cuh`
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, the function signatures in the Spectral clustering group are rendered differently than other functions (all highlighted with bold mangenta colored text). Note that is not caused by this PR, it was like that previously.
image

:project: RAFT




mdarray
#######
Copy link
Contributor

Choose a reason for hiding this comment

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

Add header file name?

@@ -136,42 +153,15 @@ mdspan
.. doxygentypedef:: raft::mdspan
Copy link
Contributor

Choose a reason for hiding this comment

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

Add header file name?



IVF-Flat
--------

Header: `raft/neighbors/ivf_flat.cuh`
Copy link
Contributor

Choose a reason for hiding this comment

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

  • The docstring of index_params and search params refers to headers as #include <ivf_flat_types.hpp>. We should have the full path, and preferably the same visual highlighting as for other headers.
  • The index_params / search params arguments of build/search is not hyperlinked to the type definition.



IVF-PQ
--------

Header: `raft/neighbors/ivf_pq.cuh`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, the header file for index and search params shall contain full path. It would be good to list these structs before the

@@ -2,8 +2,6 @@
C++ API Reference
~~~~~~~~~~~~~~~~~



Copy link
Contributor

Choose a reason for hiding this comment

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

Shall the section heads below be in alphabetical order?


Brute-force
-----------

Header: `raft/neighbors/brute_force.cuh`
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding a cross reference to sparse/neighbors/brute_force_knn?

@cjnolet
Copy link
Member Author

cjnolet commented Nov 23, 2022

@tfeher thank you for the detailed review (and for doing it on such short notice)! Since code freeze is tomorrow (which is a US holiday), I’m definitely going to create a follow-on PR for all of these items, but I’ll target it for 23.02.

@cjnolet
Copy link
Member Author

cjnolet commented Nov 24, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 11c5105 into rapidsai:branch-22.12 Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
Development

Successfully merging this pull request may close these issues.

3 participants