Skip to content

Fix forwarding of initial_alpha and learning_rate in UMAP #6417

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2025
Merged

Fix forwarding of initial_alpha and learning_rate in UMAP #6417

merged 1 commit into from
Mar 11, 2025

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented Mar 11, 2025

Previously these parameters weren't fully forwarded properly to libcuml.

Split out from #6316 (thanks Victor!)

@jcrist
Fix forwarding of initial_alpha and learning_rate in UMAP
4c64b9a 
Previously these parameters weren't fully forwarded properly to
`libcuml`.
@jcrist jcrist requested a review from a team as a code owner March 11, 2025 03:26
@jcrist jcrist requested review from csadorf and wphicks March 11, 2025 03:26
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Mar 11, 2025
@jcrist jcrist added bug Something isn't working non-breaking Non-breaking change labels Mar 11, 2025
This was referenced Mar 11, 2025
Open
Closed
csadorf
csadorf approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

I would recommend to add some inline comments to explain the relationship between .learning_rate and ._initial_alpha if possible, otherwise LGTM.

@jcrist
Copy link
Member Author

jcrist commented Mar 11, 2025

Ah, that explanation makes sense. Will add comments in a follow-up.

@jcrist
Copy link
Member Author

jcrist commented Mar 11, 2025

/merge

@rapids-bot rapids-bot bot merged commit 7065123 into rapidsai:branch-25.04 Mar 11, 2025
67 of 70 checks passed
@jcrist jcrist deleted the fixup-umap-params branch March 11, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viclafargue viclafargue viclafargue left review comments

@csadorf csadorf csadorf approved these changes

@wphicks wphicks Awaiting requested review from wphicks wphicks is a code owner automatically assigned from rapidsai/cuml-python-codeowners

Assignees
No one assigned
Labels
bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jcrist @csadorf @viclafargue