[DOC] Add missing parameters to UMAP and NearestNeighbors docstrings#7632
[DOC] Add missing parameters to UMAP and NearestNeighbors docstrings#7632rapids-bot[bot] merged 15 commits intorapidsai:mainfrom
Conversation
|
I think the DOC label is missing, I can not change the label myself. |
|
Thanks for adding the doc label. I think the last step to run the pipeline successful is to mark the change as non-breaking because it's just an improvement in the documentation. |
|
Please rebase this PR and target the |
jinsolp
left a comment
There was a problem hiding this comment.
Thank you for your work! Added a minor suggestion.
| (e.g. for a regression problem) then metric of 'l1' or 'l2' is | ||
| probably more appropriate. |
There was a problem hiding this comment.
| (e.g. for a regression problem) then metric of 'l1' or 'l2' is | |
| probably more appropriate. | |
| (e.g. for a regression problem) then metric of 'euclidean' or 'l2' is | |
| probably more appropriate. |
Suggesting small fix because our supported target metrics are defined as below;
_TARGET_METRICS = {
"euclidean": lib.MetricType.EUCLIDEAN,
"l2": lib.MetricType.EUCLIDEAN,
"categorical": lib.MetricType.CATEGORICAL,
}
…arget_n_neighbors, target_weight, target_metric) and Nearest Neighbors, for NN I added sqeuclidean, inner_product, jaccard, hellinger, haversine for the metric, because according to the code it is supported. ; added parameter documentation.
…cumentation of umap-learn
3e3c3ab to
82c0e02
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I've rebased the branch and updated the PR target branch. |
|
/ok to test c9caf7f |
No, this should be good to go. We just need to get a good CI run. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughUMAP gains three public parameters— Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@python/cuml/cuml/neighbors/nearest_neighbors.pyx`:
- Around line 475-488: The docstring for metric in nearest_neighbors.pyx has a
missing opening quote before correlation in the metric list causing a mismatched
quote; update the metric string list (in the nearest_neighbors.pyx docstring
block where "metric : string..." is defined) to add the missing leading quote so
that 'correlation' is properly quoted and the sentence reads "'jensenshannon',
'cosine', 'braycurtis', 'jaccard', 'hellinger', 'correlation', 'inner_product'."
|
/ok to test cce3ed2 |
|
@csadorf okay nice. I will take a closer look on the weekend if the CI is not working until then. |
|
/ok to test 0fc5965 |
I think what happened is that #7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in #7632 is that #7751 was merged before #7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: #7768
|
/ok to test 6ed6b51 |
|
/merge |
I think what happened is that rapidsai#7751 fixed something that used to lead to the `check_do_not_raise_errors_in_init_or_set_params` check failing. The reason we ended up seeing it in rapidsai#7632 is that rapidsai#7751 was merged before rapidsai#7753 (and we didnt rerun the CI for that PR). Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) URL: rapidsai#7768
…apidsai#7632) **Related issue:** Issue rapidsai#6211 I added the requested documentation for UMAP and Nearest Neighbor. **UMAP:** Added documentation for the parameters `target_n_neighbors`, `target_weight`, `target_metric`. Except for some typos I took the documentation from umap-learn. **Nearest Neighbor:** I added the supported `metrics` as requested in the rapidsai#6211 issue, and double checked them with the code if they are supported. This is my first contribution, if everything is okay I would look into another issue. Authors: - Silas Müller (https://github.com/silasmue) - Simon Adorf (https://github.com/csadorf) Approvers: - Simon Adorf (https://github.com/csadorf) - Jinsol Park (https://github.com/jinsolp) URL: rapidsai#7632
Related issue: Issue #6211
I added the requested documentation for UMAP and Nearest Neighbor.
UMAP: Added documentation for the parameters
target_n_neighbors,target_weight,target_metric. Except for some typos I took the documentation from umap-learn.Nearest Neighbor: I added the supported
metricsas requested in the #6211 issue, and double checked them with the code if they are supported.This is my first contribution, if everything is okay I would look into another issue.