-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add draft of Dask NEP-18 advances #15
Conversation
@mrocklin let me know what you think of this. |
cc also @jakirkham |
I think it's a fine blogpost. Some large scale comments/questions:
|
Thanks for the comments, replying:
|
Dask adds some overhead. I think it's worth trying out NumPy in isolation as well.
I don't think that users care. All's fair in love, war, and performance. Scikit-Learn uses solvers that are asymptotically better than those used in dask-glm. Dask-GLM's solvers were designed to scale well, but they're not particularly good on a single node.
Well, there are larger scale things here that I think are very exciting. You've gotten a pre-existing codebase to mostly work on GPUs without doing that much work. From an ecosystem perspective that's pretty interesting. |
I didn't mean to say exclusively about compute performance, but just in general, perhaps quality of results could be very much different, and I was trying to avoid leaving an untruthful conclusion for the reader based on information I don't currently have, for example: "CuPy is slower than Scikit-Learn, thus it must mean it's quality is much higher".
Ok, so you're talking more from a design perspective now, indeed in that you're right. I was thinking more from performance and application points of view. I will then have to rethink a bit what and how to write it to emphasize that perspective. |
Sure, you can give the context above, saying that Scikit-Learn just uses better algorithms that converge more quickly . You may also want to read through: http://blog.dask.org/2017/03/22/dask-glm-1 |
Alright, I updated this, please tell me what you think @mrocklin. |
Looking forward to reading through this. Also, cc @jakirkham for review |
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 feedback.
In general this looks great to me. The examples are very motivating and it's a fun read.
I could be wrong, but I don't think the estimators have changed
substantially in dask-ml:
https://github.com/dask/dask-ml/commits/master/dask_ml/linear_model/glm.py
…On Wed, Mar 13, 2019 at 10:58 AM Peter Andreas Entschev < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In _posts/2019-03-11-dask-nep18.md
<#15 (comment)>:
> +
+```
+Solver admm with Dask took 22444.307 ms to fit and 41.299 ms to predict
+Solver admm with CuPy took 263.742 ms to fit and 1.146 ms to predict
+
+Solver lbfgs with Dask took 1561.047 ms to fit and 41.063 ms to predict
+Solver lbfgs with CuPy took 13.935 ms to fit and 0.260 ms to predict
+
+Solver newton with Dask took 785.150 ms to fit and 42.822 ms to predict
+Solver newton with CuPy took 22.480 ms to fit and 0.248 ms to predict
+
+Solver proximal_grad with Dask took 1902.620 ms to fit and 45.349 ms to predict
+Solver proximal_grad with CuPy took 11.700 ms to fit and 0.257 ms to predict
+
+Solver gradient_descent with Dask took 2483.603 ms to fit and 42.605 ms to predict
+Solver gradient_descent with CuPy took 14.580 ms to fit and 0.257 ms to predict
Also, Dask-GLM estimators were deprecated in dask/dask-glm#66
<dask/dask-glm#66>. Maybe they indeed have a bug
which may or may not have been fixed in Dask-ML.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#15 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABQHItQ2dv_3PnjUfKfrsBl1DBzZ3cxlks5vWSAIgaJpZM4bgx_W>
.
|
@TomAugspurger that's possible, I'll try to investigate causes later, but it's probably useful to try out those in Dask-ML too. |
For the time being, I don't plan any more changes. Ready for another review. |
I think that this looks great. The one suggestion I would make is in the axes of the timing plots. The main point of these plots seems to be to compare the relative performance between NumPy and CuPy. To this end, it would be useful to have the NumPy and CuPy performance numbers on the same axes, so that one can see visually when the CuPy numbers are higher or lower than NumPy's numbers. Currrently they look visually similar at first until you look at the numbers on the y-axes. I could imagine rearranging the plots so that there is a different plot per solver, throwing all of the solvers into one plot, or keeping the two separate plots, but fixing the axes of the second plot to match the axes of the first. |
(I'm also happy to just push the merge button, this is great as it is) |
@mrocklin I think the split by solver is a better solution. When first plotting the graphs I thought it would be too cluttered, but I did it now and it looks fine to me. So if there's no more comments, from my side it's ok to merge. |
Those plots do look nice :) |
I plan to merge this PR in an hour if there are no objections. |
I've merged this in and it's live. It's a bit late in the week though to start publicizing it. I'll tweet about it early next week. |
Now that you've mentioned it, maybe we should have waited until Monday to publish that. Well, too late now I guess. :) |
I've just made it a draft, which should keep it off of the TOC for now. |
Nice, please publish it at the most appropriate time, you probably have experience in knowing which time is best, and I don't. |
Thanks @pentschev. This is really great. I read an earlier version of this and really like the improvements you have made since. @mrocklin and I played with a benchmark based on your work earlier this week and it really shows the improvements you have made here! 😄 Sorry I was late to review. Have submitted some small tweaks to the text in PR ( #18 ). These are mostly minor syntax changes. Though please take a look and make sure I haven't lost something in the changes. As another note, it would be nice to list the BLAS and LAPACK library used with NumPy for the CPU case (can just run |
No description provided.