DOC fix DecisionBoundaryDisplay color consistency in plot_scaling_importance example#33654
DOC fix DecisionBoundaryDisplay color consistency in plot_scaling_importance example#33654yejiming wants to merge 1 commit intoscikit-learn:mainfrom
Conversation
ogrisel
left a comment
There was a problem hiding this comment.
Thanks for the PR. I think we can simplify the code a bit though. The use of mpl.colors.ListedColormap does not seem to be necessary in particular.
| cmap = mpl.colors.ListedColormap(disp.multiclass_colors_) | ||
| disp.ax_.scatter( | ||
| X_plot["proline"], | ||
| X_plot["hue"], | ||
| c=y, | ||
| cmap=cmap, | ||
| s=20, | ||
| edgecolor="k", | ||
| ) |
There was a problem hiding this comment.
| cmap = mpl.colors.ListedColormap(disp.multiclass_colors_) | |
| disp.ax_.scatter( | |
| X_plot["proline"], | |
| X_plot["hue"], | |
| c=y, | |
| cmap=cmap, | |
| s=20, | |
| edgecolor="k", | |
| ) | |
| disp.ax_.scatter( | |
| X_plot["proline"], X_plot["hue"], c=y, cmap=cmap, s=20, edgecolor="k", | |
| ) |
There was a problem hiding this comment.
The use of mpl.colors.ListedColormap will be needed again with the new approach.
| X_plot, | ||
| response_method="predict", | ||
| alpha=0.5, | ||
| multiclass_colors=multiclass_cmap, |
There was a problem hiding this comment.
| multiclass_colors=multiclass_cmap, | |
| multiclass_colors=cmap, |
There was a problem hiding this comment.
Actually, this could even be simplified to
| multiclass_colors=multiclass_cmap, | |
| multiclass_colors="viridis" |
and the other two code changes could be removed, as viridis is the default for scatter anyways.
There was a problem hiding this comment.
This can then be removed to use the default colors.
| X_plot = X[["proline", "hue"]] | ||
| X_plot_scaled = scaler.fit_transform(X_plot) | ||
| clf = KNeighborsClassifier(n_neighbors=20) | ||
| multiclass_cmap = "viridis" |
There was a problem hiding this comment.
| multiclass_cmap = "viridis" | |
| cmap = "viridis" |
There was a problem hiding this comment.
This will no longer be needed.
ogrisel
left a comment
There was a problem hiding this comment.
My understanding is that if we stick to the use of a continuous colormap such as "viridis", we don't need the extra complexity of the mpl.colors.ListedColormap.
We would only need this if we were to change this example to use a qualitative colormap. However as recently discussed in #33115 (comment), the default "tab10" qualitative color cycle is not colorblind-friendly (even for the first 3 colors), so unless we find a better qualitative color cycle for the 3 colors case, I am fine with using viridis for now.
| # boundary that is much worse in comparison to a model trained on the full set | ||
| # of features. | ||
|
|
||
| import matplotlib as mpl |
There was a problem hiding this comment.
| import matplotlib as mpl |
There was a problem hiding this comment.
This will be needed to enable cmap = mpl.colors.ListedColormap(disp.multiclass_colors_)
|
@ogrisel, could you please close this one since there was no response from the author since it was opened three weeks ago? I'll fix this myself. |
Reference Issues/PRs
Towards #33557
What does this implement/fix? Explain your changes.
This PR makes the multiclass colors consistent between the
DecisionBoundaryDisplaybackground and the scatter plot inexamples/preprocessing/plot_scaling_importance.py.It also cleans up a few small wording issues in the same example while keeping
the scope limited to this file.
I checked the rendered example locally with
EXAMPLES_PATTERN="plot_scaling_importance" make htmlfrom the
doc/directory.AI usage disclosure
I used AI assistance for: