[MRG+1] Refactoring plot_iris svm example. #8279
Conversation
|
@ogrisel, it looks like you were the last one to do a similar revision to this example. |
| Parameters | ||
| ---------- | ||
| ax: matplotlib axes object | ||
| clf: a classifier |
|
@lemonlaug Is-it possible to add black contours at each circle ? |
|
@TomDLT That's a default style change in matplotlib 2.0, fixed. |
Codecov Report
@@ Coverage Diff @@
## master #8279 +/- ##
=========================================
Coverage ? 94.75%
=========================================
Files ? 342
Lines ? 60801
Branches ? 0
=========================================
Hits ? 57609
Misses ? 3192
Partials ? 0Continue to review full report at Codecov.
|
|
@lemonlaug Perfect :) LGTM |
examples/svm/plot_iris.py
Outdated
| point_params: dictionary of params to pass to `ax.scatter`, optional | ||
| countour_params: dictionary of params to pass to `plot_contours`, optional | ||
| """ | ||
| contours = plot_contours(ax, clf, xx, yy, **contour_params) |
There was a problem hiding this comment.
It seems a bit silly to have a shared function for contour and point plotting, if their arguments are disjoint and both are delegated to other functions.
There was a problem hiding this comment.
Yes, I also waffled over this. I think my initial impulse was to try and get all of the plotting concerns separated into functions, but given that it ultimately doesn't make sense for the axis formatting, I agree that maybe this is a step too far.
There was a problem hiding this comment.
And I now see more clearly that the setting of xlim and ylim doesn't belong in the contour function.
…moving set_*lim calls outside decision boundary function.
|
@jnothman Did those changes address your concerns? I'm not sure what the failure from codecov is complaining about. Would love a tip from anyone on how to address that. |
|
LGTM, thanks! |
What does this implement/fix? Explain your changes.
This refactors without in any way changing the outcome of the code in the "Plot different SVM classifiers in the iris dataset" example.
A couple of features of the current state of affairs that are motivating:
enumeratewherezipwould be more appropriate.X, fitting, restating names of models.matplotlibstate machine interface rather than the object-oriented interface and stretches that interface about as far as it ought to be stretched, making the code unsuitable to serve as the basis for extension or reuse.I've addressed the concerns by switching to OO interface for matplotlib and by trying to follow some best practices, most notably by using the function signature recommended by matplotlib.
Any other comments?
I think this is important, because it's one of the chief ways people learn about both
sklearnandmatplotlib, so demonstrating best practices can have a positive effect for people with limited exposure to both packages--those who are most likely to be cribbing from examples.If this is considered to be an improvement, would love to give more of the examples the same treatment.