Skip to content
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

Linear svr #65

Closed
wants to merge 28 commits into from
Closed

Linear svr #65

wants to merge 28 commits into from

Conversation

210057zzh
Copy link
Contributor

@210057zzh 210057zzh commented Oct 28, 2022

Description

Fixes #33

Add LinearSVR

Contribution Checklist

If your contribution modifies code in the core library (not docs, tests, or examples), please fill the following checklist.

  • My contribution modifies code in the main library.
  • My modifications are tested.
  • My modifications are documented.

Copy link
Member

@seba-1511 seba-1511 left a comment

Choose a reason for hiding this comment

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

Thanks @210057zzh, it's a really good start. There are a few rough edges in the fit() implementation, I'll bring that up in our next meeting.

end = time.time()
# print(end - start)
start = time.time()
reflsvc = svm.LinearSVC(max_iter=100000)
Copy link
Member

Choose a reason for hiding this comment

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

The iterations are not the same for torchml and sklearn?

lsvc = LinearSVC(max_iter=1000)
start = time.time()
lsvc.fit(torch.from_numpy(x), torch.from_numpy(y))
end = time.time()
Copy link
Member

Choose a reason for hiding this comment

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

Remove timings, or use them in the tests (eg, to make sure our implementation is fast enough).

end = time.time()
print(end - start)
start = time.time()
reflsvr = svm.LinearSVR(max_iter=100000)
Copy link
Member

Choose a reason for hiding this comment

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

  • Different max_iter?
  • Remove or use timings.

Comment on lines 61 to 73
"""
Predict class labels for samples in X.

Parameters
----------
X : {array-like, sparse matrix} of shape (n_samples, n_features)
The data matrix for which we want to get the predictions.

Returns
-------
y_pred : ndarray of shape (n_samples,)
Vector containing the class labels for each sample.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Wrong docstring format.

Comment on lines +24 to +39
super(LinearSVC, self).__init__()
self.coef_ = None
self.intercept_ = None
self.classes_ = None
self.dual = dual
self.tol = tol
self.C = C
self.multi_class = multi_class
self.fit_intercept = fit_intercept
self.intercept_scaling = intercept_scaling
self.class_weight = class_weight
self.verbose = verbose
self.random_state = random_state
self.max_iter = max_iter
self.penalty = penalty
self.loss = loss
Copy link
Member

Choose a reason for hiding this comment

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

Do we support all values for all arguments? If not, can we raise exceptions for the values we don't support?

Comment on lines 119 to 120
X_param.value = X.numpy()
y_param.value = y.numpy()
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need .value to be a numpy array?

C_param.value = self.C
prob.solve(solver="ECOS", abstol=self.tol, max_iters=self.max_iter)

self.coef_ = torch.cat((self.coef_, torch.t(torch.from_numpy(w.value))))
Copy link
Member

Choose a reason for hiding this comment

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

Here too: can we just use the torch Tensor w instead of numpy? (And below for the bias too)


Initialize the class with training sets

## Arguments
Copy link
Member

Choose a reason for hiding this comment

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

New line after ##

import cvxpy as cp


class LinearSVC(ml.Model):
Copy link
Member

Choose a reason for hiding this comment

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

Docstring?

constraints = []

prob = cp.Problem(cp.Minimize(objective), constraints)
X_param.value = X.numpy()
Copy link
Member

Choose a reason for hiding this comment

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

Also here: do we need the .numpy()?

@seba-1511 seba-1511 mentioned this pull request Nov 4, 2022
3 tasks
@210057zzh 210057zzh closed this Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinearSVR
2 participants