-
Notifications
You must be signed in to change notification settings - Fork 86
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
Improving getting started materials #342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-24.10 #342 +/- ##
=============================================
Coverage 70.37% 70.37%
=============================================
Files 12 12
Lines 54 54
=============================================
Hits 38 38
Misses 16 16 ☔ View full report in Codecov by Sentry. |
…oc-2410-index_docs
…oc-2410-index_docs
Co-authored-by: Micka <mide@nvidia.com>
…oc-2410-index_docs
Moved the cpp tutorial over to a differerent branch for now: https://github.com/rapidsai/cuvs/tree/doc-2412-cpp_tutorial |
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.
Overall things look fantastic! Just very few minor comments
|
||
```bash | ||
mamba install -c conda-forge -c nvidia -c rapidsai cuvs | ||
conda install -c conda-forge -c nvidia -c rapidsai cuvs |
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.
Would be great to also add the pip command below this.
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.
The only reason I didn't is to try and minimize the number of places we have this info, since it adds to the challenge of keeping everything up to day. I think we should definiely mention pip, though, and link to the build guide. I considered doing this for conda too. What do you think? Or as a user, would you rather see both pip and conda right in the readme?
README.md
Outdated
``` | ||
|
||
Please see the [Build and Install Guide](https://docs.rapids.ai/api/cuvs/stable/build/) for more information on installing cuVS and building from source. | ||
Please see the [Build and Install Guide](https://docs.rapids.ai/api/cuvs/nightly/build/) for more information on installing cuVS and building from source. |
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.
Consideration for follow up: it'd be great to have a script or job that changes this links to stable
at release time, and keeps the as nightly
during dev
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.
amazing work on this - this is a great change.
I left some minor comments, but pre-approving since none of them are blocking
/merge |
Adding documentation for each index type. This decouples the major details in the docs from the individual language API docs to present users with a better overal experience. The major topcs in the new index-level docs are meant to be more exhaustive than the API docs in providing