[MRG+1]Update discriminant analysis code for better memory usage #10904
Conversation
There was a problem hiding this comment.
Thanks for this PR @bobchennan !
Please add [MRG] at the beginning of the PR title if this is ready for review. A few comments are below.
What's the estimated memory usage gain from this change? (you can measure memory usage, for instance, with memory_profiler.
sklearn/discriminant_analysis.py
Outdated
| covs.append(np.atleast_2d(_cov(Xg, shrinkage))) | ||
| return np.average(covs, axis=0, weights=priors) | ||
| if priors is None: | ||
| cov = cov + np.atleast_2d(_cov(Xg, shrinkage)) / len(classes) |
There was a problem hiding this comment.
Since cov is already allocated, you could just do an inplace add cov += ..
sklearn/discriminant_analysis.py
Outdated
| if priors is None: | ||
| cov = cov + np.atleast_2d(_cov(Xg, shrinkage)) / len(classes) | ||
| else: | ||
| cov = cov + priors[idx] * np.atleast_2d(_cov(Xg, shrinkage)) |
There was a problem hiding this comment.
I'm not familiar with this part of the code base, but if I understand correctly #10898 was only about memory usage and I don't see how this line can be equivalent to the one above (if priors=None).
sklearn/discriminant_analysis.py
Outdated
| classes = np.unique(y) | ||
| for group in classes: | ||
| means = np.empty(shape=(len(classes), X.shape[1]), dtype=X.dtype) | ||
| for idx, group in enumerate(classes): |
There was a problem hiding this comment.
This looks good, though fwiw, I think we could do this without a loop, using no.add.at
There was a problem hiding this comment.
No sure I understand this part. I can re-write it as
means = np.array([X[y==group].mean(0) for group in classes])
but the loop is still inside.
sklearn/discriminant_analysis.py
Outdated
| if priors is None: | ||
| cov = cov + np.atleast_2d(_cov(Xg, shrinkage)) / len(classes) | ||
| else: | ||
| cov = cov + priors[idx] * np.atleast_2d(_cov(Xg, shrinkage)) |
There was a problem hiding this comment.
Please avoid this duplication, by only doing the if for the prior, not the whole statement.
You also make the assumption that priors add to 1. The previous version did not. Please verify that this is a safe assumption.
|
Regarding to the sum of priors, in fit function it is already assigned (thus not None) and normalized. |
bobchennan
changed the title
|
For the memory usage I will give an example later. |
sklearn/discriminant_analysis.py
Outdated
| classes, ny = np.unique(y, return_inverse=True) | ||
| cnt = np.bincount(ny) | ||
| means = np.zeros(shape=(len(classes), X.shape[1])) | ||
| for idx in xrange(X.shape[0]): |
There was a problem hiding this comment.
This loop over n_samples is slow.
What about the equivalent array operation:
means = np.zeros(shape=(len(classes), X.shape[1]))
np.add.at(means, ny, X)
means /= cnt[:, None]
return meansThere was a problem hiding this comment.
Looks good! I don't know this function before.
| Xg = X[y == group, :] | ||
| covs.append(np.atleast_2d(_cov(Xg, shrinkage))) | ||
| return np.average(covs, axis=0, weights=priors) | ||
| cov += priors[idx] * np.atleast_2d(_cov(Xg, shrinkage)) |
There was a problem hiding this comment.
This line does not work if priors is None. You should do something like:
cov_g = np.atleast_2d(_cov(Xg, shrinkage))
if priors is not None:
cov_g *= priors[idx]
cov += cov_gThere was a problem hiding this comment.
Oh priors cannot be None.
Then maybe we should remove the default which implies None is a valid input.
| covs.append(np.atleast_2d(_cov(Xg, shrinkage))) | ||
| return np.average(covs, axis=0, weights=priors) | ||
| cov += priors[idx] * np.atleast_2d(_cov(Xg, shrinkage)) | ||
| return cov |
There was a problem hiding this comment.
In your version, you don't return the weighted average as before, but the weighted sum.
You should divide cov by the sum of priors, or by n_classes if the priors are None.
There was a problem hiding this comment.
Ok you answered it already, my mistake
|
One example of memory usage is given here. |
TomDLT
changed the title
sklearn/discriminant_analysis.py
Outdated
| Xg = X[y == group, :] | ||
| means.append(Xg.mean(0)) | ||
| return np.asarray(means) | ||
| classes, ny = np.unique(y, return_inverse=True) |
There was a problem hiding this comment.
I don't know what ny means. Perhaps just overwrite y?
|
Please add an entry to the Enhancements change log at |
|
@rth any suggestions? |
|
Thank you @bobchennan ! |
Fixed #10898