Conversation
This reverts commit d541aeb.
jeanfeydy
left a comment
jeanfeydy
left a comment
There was a problem hiding this comment.
Fine! The KNN code is now 99% complete, but there remains quite a lot of work to be done on the Nystroem side. We'll talk about it tomorrow: see you soon :-)
pykeops/common/nystrom_generic.py
Outdated
| GenericLazyTensor = TypeVar("GenericLazyTensor") | ||
|
|
||
|
|
||
| class GenericNystrom: |
There was a problem hiding this comment.
Following the scikit-learn convention, we should probably call everything "Nystroem" instead of "Nystrom" (from classes to file names).
pykeops/common/nystrom_generic.py
Outdated
| n_components = how many samples to select from data. | ||
| kernel = type of kernel to use. Current options = {rbf:Gaussian, | ||
| exp: exponential}. | ||
| sigma = exponential constant for the RBF and exponential kernels. | ||
| eps = size for square bins in block-sparse preprocessing. | ||
| k_means = number of centroids for KMeans algorithm in block-sparse | ||
| preprocessing. | ||
| n_iter = number of iterations for KMeans. | ||
| dtype = type of data: np.float32 or np.float64 | ||
| inv_eps = additive invertibility constant for matrix decomposition. | ||
| verbose = set True to print details. | ||
| random_state = to set a random seed for the random sampling of the samples. | ||
| To be used when reproducibility is needed. |
There was a problem hiding this comment.
Please note that we follow the Google docstring convention, which is documented here.
pykeops/common/nystrom_generic.py
Outdated
| self.tools = None | ||
| self.LazyTensor = None | ||
|
|
||
| self.device = "cuda" if pykeops.config.gpu_available else "cpu" |
There was a problem hiding this comment.
Shouldn't we use the device that is used originally by the input data? I don't know how self.device = "cuda" would handle multi-GPU configurations.
pykeops/common/nystrom_generic.py
Outdated
| self.verbose = verbose | ||
| self.random_state = random_state | ||
| self.tools = None | ||
| self.LazyTensor = None |
There was a problem hiding this comment.
As detailed in the previous review, I think that using self.LazyTensor as an attribute name is a bit dangerous. You could use e.g. self.lazy_tensor instead?
pykeops/common/nystrom_generic.py
Outdated
| # Set default sigma | ||
| # if self.sigma is None and self.kernel == 'rbf': | ||
| if self.sigma is None: | ||
| self.sigma = np.sqrt(x.shape[1]) |
There was a problem hiding this comment.
Is this the scikit-learn default? I would have expected a set value, or something like 10% of the diameter of the data.
| self.K_nq = K_nq # dim: number of samples x num_components | ||
| self.K_nq.backend = "GPU_2D" | ||
| self.normalization = normalization |
There was a problem hiding this comment.
As with NumPy, "GPU_2D" should only be applied on the "transposed" term.
| x_LT = LazyTensor(x.unsqueeze(0).to(device)) | ||
| y_LT = LazyTensor(y.unsqueeze(1).to(device)) | ||
| d = ((x_LT - y_LT) ** 2).sum(-1) | ||
| true_nn = d.argKmin(K=k, dim=1).long() |
There was a problem hiding this comment.
For the sake of consistency, it would be better to use the names x_i, y_j, D_ij, indices and add the expected array shapes in the comments.
| def accuracy(indices_test, indices_truth): | ||
| """ | ||
| Compares the test and ground truth indices (rows = KNN for each point in dataset) | ||
| Returns accuracy: proportion of correct nearest neighbours | ||
| """ | ||
| N, k = indices_test.shape | ||
|
|
||
| # Calculate number of correct nearest neighbours | ||
| accuracy = 0 | ||
| for i in range(k): | ||
| accuracy += torch.sum(indices_test == indices_truth).float() / N | ||
| indices_truth = torch.roll( | ||
| indices_truth, 1, -1 | ||
| ) # Create a rolling window (index positions may not match) | ||
| accuracy = float(accuracy / k) # percentage accuracy | ||
|
|
||
| return accuracy |
There was a problem hiding this comment.
Since we use this function in several tutorials, shouldn't we factor it somewhere in the utils module?
| x_LT = LazyTensor(x.unsqueeze(0).to(device)) | ||
| y_LT = LazyTensor(y.unsqueeze(1).to(device)) | ||
| d = ((x_LT - y_LT) ** 2).sum(-1) | ||
| true_nn = d.argKmin(K=k, dim=1).long() |
There was a problem hiding this comment.
Since this code is used above, shouldn't we factor it in a function to avoid copy-pastes?
| ######################################################################## | ||
| # NNDescent search using clusters and Manhattan distance | ||
| # Second experiment with N=$10^6$ points in dimension D=3, with 5 nearest neighbors and manhattan distance. |
There was a problem hiding this comment.
Couldn't we factor the timings/display code above in a function, and call it again with a different metric function instead of copy-pasting it below?
added accuracy to torch tools
added accuracy to torch tools
shifted accuracy to torch.utils
shifted accuracy to torch.utils
factorized timing function
5 participants
First request