-
Notifications
You must be signed in to change notification settings - Fork 0
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
FEA ridge benchmarks #18
Conversation
d0720da
to
8f06185
Compare
I'm not quite confident I wrote the objective evaluation well. Is that correct ? if self.sample_weight_ is not None:
X, y, _ = _rescale_data(X, y, self.sample_weight_, inplace=False)
y = y.reshape((y.shape[0], -1))
weights = weights.reshape((-1, X.shape[1], 1))
value = (
(((X @ weights).squeeze(2) + (intercept - y).T) ** 2).sum()
+ (self.alpha * (weights**2).sum())
) / (X.shape[0] * len(y.T)) or is it better to just report r2_score ? |
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.
Some feed back.
What you wrote seems sensible at first glance but would require detailed code inspection to check that this is actually what is optimized in scikit-learn internally. Note that I started work to improve the convergence tests for Ridge models scikit-learn/scikit-learn#25948 but I could not yet find time to get back to it. |
Co-authored-by: Olivier Grisel <[email protected]>
…ers of different nature ?
So I added some solvers other than svd. Not sure it's appropriate though, since the initial purpose is rather comparing backends of same solvers. If we want to have different solvers, then maybe let's consider having one different folder and sheet per solver. I think it's still interesting to look at but more difficult to setup and read (because of max_iter and tol parameters mostly) |
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.
I browsed the results a bit and it's quite interesting.
Based on the results, here are some suggestions to the benchmark setting to make this even more interesting (in my opinion).
benchmarks/ridge/objective.py
Outdated
("lsqr", 25, 0), | ||
("sparse_cg", 25, 0), | ||
("sag", 50, 0), | ||
("saga", 25, 0), |
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.
max_iter
below 100 seems small to me. Based on the results it happens quite to often that the maximum is reached for lsqr
, possibly with a very bad value of the objective.
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.
I reverted to None and set tol to 1e-4 (which are the defaults parameters), so iteration-based solvers might have different number of iterations now. For multi targets, lsqr report a different number of iterations for each target, but it's unconvenient to report all those... I settled for report of the maximum over the targets, after observing that it doesn't seem to differ much from one target to another.
I have updated following your suggestions +sync'd the spreadsheet, but for some reason I can't run the cuml solver anymore:
edit: so apparently cuml svd solver uses a lot of vram and this is a vram issue. I add some more dataset dimensions that show what cuml supports or not. (no issue with torch+cuda svd on the other hand) edit2: does not explain all occurences of the error edit3: cuml svd solver does not accept n_features > n_samples apparently, and use up a lot of memory, will crash on biggest datasets. |
Somes fixes done, and all results pushed and synchronized to the sheet. Let's merge ? |
Targeted devices will be 32 cores cpu and cuda gpu. Maybe intel max series too if access is not too complicated.
Probably a follow-up PR: