-
Notifications
You must be signed in to change notification settings - Fork 30
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
[LogisticRegression] Support standardization for dense vectors #565
Conversation
Signed-off-by: Jinfeng <[email protected]>
Signed-off-by: Jinfeng <[email protected]>
…-memory access bug in sum kernal
f713a77
to
b0d772f
Compare
build |
init_parameters["fit_intercept"] is True | ||
and len(intercept_array) > 1 | ||
): | ||
intercept_mean = sum(intercept_array) / len(intercept_array) |
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.
Is there a 'mean' method that can be called? Also, how does this not change the model output?
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.
Yeah, revised the code to use np.mean and cp.mean
convert_to_sparse: bool = False, | ||
) -> LogisticRegression: | ||
assert ( | ||
standardization is False | ||
), "standardization=True is not supported due to testing with single-GPU LogisticRegression" |
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.
Can we use cuml standard scalar preprocessr?
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.
revised the code to remove the standardization option from argument list.
this test was for standardization=False only.
build |
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.
Nice. Just one more question.
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.
👍
Sparse vectors will be densified to dense vectors if standardization is on.
The PR removes uvm in the test of nlp20news dataset, because it is found the uvm leads to a cuda memset error in vars calculation.