Skip to content

MNT refactor _rescale_data in linear models into _preprocess_data #31418

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

lorentzenchr
Copy link
Member

@lorentzenchr lorentzenchr commented May 23, 2025

Reference Issues/PRs

None

What does this implement/fix? Explain your changes.

Cleanup of some legacy maintenance burden which makes the code more readable - hopefully.

Any other comments?

The logic of computation should be identical - unless I made a mistake.
This should also make it easier to remove the X_scale - again a legacy burden left as # TODO.

Copy link

github-actions bot commented May 23, 2025

✔️ Linting Passed

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

Generated for commit: 7ae36cb. Link to the linter CI: here

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @lorentzenchr. I made a pass myself and I'm also confident that you didn't change existing computations. Here are some comments

X,
y,
fit_intercept=self.fit_intercept,
copy=self.copy_X,
sample_weight=sample_weight,
rescale_with_sw=True,
Copy link
Member

Choose a reason for hiding this comment

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

it's the default

Suggested change
rescale_with_sw=True,

unless you wanted to make it extra explicit ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was my intent. While reviewing code, I usually don't want to know by hard every default value of private functions.

* ValueError: non-broadcastable output operand with shape () doesn't match the broadcast shape (1,)
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @lorentzenchr

@jeremiedbb jeremiedbb added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jul 28, 2025
@lorentzenchr
Copy link
Member Author

@jeremiedbb Thanks for your review.
I think with the latest government change this PR could be considered small maintenance changes without modification of code logic, so one reviewer is enough and you could merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants