-
-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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
base: main
Are you sure you want to change the base?
Conversation
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.
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, |
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.
it's the default
rescale_with_sw=True, |
unless you wanted to make it extra explicit ?
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.
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,)
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. Thanks @lorentzenchr
@jeremiedbb Thanks for your review. |
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
.