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

Enable GPU fit and CPU transform in UMAP #6032

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

divyegala
Copy link
Member

No description provided.

@divyegala divyegala added feature request New feature or request non-breaking Non-breaking change labels Aug 19, 2024
@divyegala divyegala requested a review from a team as a code owner August 19, 2024 16:29
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Aug 19, 2024
Copy link
Contributor

@viclafargue viclafargue left a comment

Choose a reason for hiding this comment

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

The code to transit from cuML to umaplib looks good. However, we should also allow regular GPU training and CPU inference without the use of pickling.

Comment on lines 870 to 872
state['_knn_dists'], state['_knn_indices'], state['_knn_search_index'] = \
nearest_neighbors(host_raw_data, self.n_neighbors, self.metric,
self.metric_kwds, False, self.random_state)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be good, but if there's a possibility to avoid running the KNN, that would be ideal.

python/cuml/cuml/manifold/umap.pyx Outdated Show resolved Hide resolved
Copy link
Contributor

@viclafargue viclafargue left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment

Comment on lines 860 to 878
@property
def _n_neighbors(self):
return self.n_neighbors

@property
def _a(self):
return self.a

@property
def _b(self):
return self.b

@property
def _initial_alpha(self):
return self.learning_rate

@property
def _disconnection_distance(self):
return DISCONNECTION_DISTANCES.get(self.metric, np.inf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't setters required to transfer attributes from umaplib to cuML?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure how it was working before then. We never had matching name conventions with them, look at _a and a for example. Is it possible we did not support cpu train/gpu infer before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think they are transfered during instantiation through hyperparameters listed in get_param_names. I do not know if we want to make these attributes writable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so we'll skip the setters then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I'm facing test errors now that these properties are part of get_attr_names. I'll need to add setters

Copy link
Contributor

@viclafargue viclafargue left a comment

Choose a reason for hiding this comment

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

LGTM

@divyegala
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 8d06238 into rapidsai:branch-24.10 Sep 9, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython / Python Cython or Python issue feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants