MRG Fix changing params test #1670
Conversation
| or name in ['CCA', 'PLSCanonical', 'PLSRegression', | ||
| 'PLSSVD', 'GaussianProcess']): | ||
| # FIXME! | ||
| # in particular GAussianProcess!!!!! |
There was a problem hiding this comment.
Typo => GaussianProcess and also avoid multiple punctuation. This codebase is a quiet working place, people need a calm environment to focus ;)
But I agree that GaussianProcess constructor params handling is very inconsistent with the rest of the code base and need a fix.
|
+1 for merging once minor comments are addressed. |
|
BTW thanks for working on this. |
|
Thank for the very quick review. I removed the exclamation marks, fixed some minor problems and force-pushed again. Hope Travis is happy now. |
|
Well @FedericoV told me that his pickle tests don't pass because the random_state is somehow altered. As I specifically added the test for this, I quitetly thought to myself |
|
Ok, build finally passed Travis. Should be good to go. |
Nice and quiet indeed :) |
| * self.random_state.normal(size=(n_features, | ||
| self.n_components))) | ||
| self.random_weights_ = (np.sqrt(self.gamma) * random_state.normal( | ||
| size=(n_features, self.n_components))) |
There was a problem hiding this comment.
More indentation please :). It's prettier.
There was a problem hiding this comment.
That would make flake8 complain, though :-/
There was a problem hiding this comment.
I can redo it a bit, though.
There was a problem hiding this comment.
The only other accepted solution is
self.random_weights_ = (np.sqrt(self.gamma)
* random_state.normal(size=
(n_features,
self.n_components)))which I dont think is very pretty
|
👍 for merge. Thanks, this is extremely useful. |
|
Merged by rebase. Thanks for the quick reviews :) |
There was a stupid bug in the test for checking that estimators don't change their
__init__params: it was only done for classifiers and clustering algos.This is a fix for regressors and transformers. I left out
GaussianProcessRegressorsince I couldn't figure out what was happening there (corrseemed to be a string, then a callable, then a number).These fixes are needed to make #1669 work. Any ideas what I should put into whatsnew?