8000 MAINT remove normalize parameter and subsequent clean-up by glemaitre · Pull Request #27855 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

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

Merged

Conversation

glemaitre
Copy link
Member
@glemaitre glemaitre commented Nov 27, 2023

Remove the parameter normalize that has been deprecating in OMP and least angle estimators.

  • remove part of code that was doing dome processing when normalize=True
  • make _preprocess_data keyword only to be more explicit in terms of parameter names.

@glemaitre glemaitre marked this pull request as draft November 27, 2023 09:38
Copy link
github-actions bot commented Nov 27, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: cc5465d. Link to the linter CI: here

@glemaitre glemaitre marked this pull request as ready for review November 27, 2023 10:30
@glemaitre glemaitre changed the title MAINT remove normalize parameter in OMP and least angle MAINT remove normalize parameter and subsequent clean-up Nov 27, 2023
@lorentzenchr
Copy link
Member

@rth @agramfort @maikia @jnothman pinging as possible reviewers, taken from #3020.

@ogrisel ogrisel self-assigned this Nov 30, 2023
@ogrisel
Copy link
Member
ogrisel commented Nov 30, 2023

The docstring of _preprocess_data needs a big update. I am working on it and will push a commit.

Copy link
Member
@ogrisel ogrisel left a 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,
*,
Copy link
Member

Choose a reason for hiding this comment

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

good!

@ogrisel ogrisel added this to the 1.4 milestone Dec 1, 2023
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])
Copy link
Member

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?

Suggested change
np.std(X, axis=0) * np.sqrt(X.shape[0])

Copy link
Member

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])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
np.std(XA, axis=0) * np.sqrt(X.shape[0])

@glemaitre
Copy link
Member Author

Thanks @lesteve for removing the left over.

Copy link
Member
@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

LGTM

@lesteve lesteve enabled auto-merge (squash) December 4, 2023 13:55
@lesteve lesteve merged commit ae2c80a into scikit-learn:main Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0