-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MAINT remove normalize parameter and subsequent clean-up #27855
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
MAINT remove normalize parameter and subsequent clean-up #27855
Conversation
@rth @agramfort @maikia @jnothman pinging as possible reviewers, taken from #3020. |
The docstring of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
We should definitely remove all the occurrences of X_scale
now that it's useless but we can do that later (it should only be private API changes and no behavioral change).
8000 | @@ -189,45 +109,51 @@ def make_dataset(X, y, sample_weight, random_state=None): | ||
def _preprocess_data( | |||
X, | |||
y, | |||
*, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good!
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@@ -415,36 +378,23 @@ def test_preprocess_data(global_random_seed): | |||
X = rng.rand(n_samples, n_features) | |||
y = rng.rand(n_samples) | |||
expected_X_mean = np.mean(X, axis=0) | |||
expected_X_scale = np.std(X, axis=0) * np.sqrt(X.shape[0]) | |||
np.std(X, axis=0) * np.sqrt(X.shape[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be removed, right?
np.std(X, axis=0) * np.sqrt(X.shape[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have pushed a commit removing this line and a similar one below
@@ -586,43 +485,30 @@ def test_sparse_preprocess_data_offsets(global_random_seed, lil_container): | |||
X = lil_container(X) | |||
y = rng.rand(n_samples) | |||
XA = X.toarray() | |||
expected_X_scale = np.std(XA, axis=0) * np.sqrt(X.shape[0]) | |||
np.std(XA, axis=0) * np.sqrt(X.shape[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np.std(XA, axis=0) * np.sqrt(X.shape[0]) |
Thanks @lesteve for removing the left over. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Remove the parameter
normalize
that has been deprecating in OMP and least angle estimators.normalize=True
_preprocess_data
keyword only to be more explicit in terms of parameter names.