8000 MNT Use name instead of float to specify colors by qinhanmin2014 · Pull Request #12199 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Use name instead of float to specify colors #12199

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 1, 2018
Merged

MNT Use name instead of float to specify colors #12199

merged 3 commits into from
Oct 1, 2018

Conversation

qinhanmin2014
Copy link
Member

FIxes #12191
I guess this is reasonable, but would definitely welcome better solution from someone more familiar with matplotlib colors

ogrisel
ogrisel approved these changes Sep 29, 2018
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I think the fix looks good to me:

  • c= is meant to pass numerical values to be mapped to a color according to a color map;
  • color= on the other hand is mean to pass a uniform color specifier.

examples/linear_model/plot_ols_ridge_variance.py Outdated
@@ -54,11 +54,11 @@
clf.fit(this_X, y_train)

ax.plot(X_test, clf.predict(X_test), color='.5')
ax.scatter(this_X, y_train, s=3, c='.5', marker='o', zorder=10)
ax.scatter(this_X, y_train, s=3, color='.5', marker='o', zorder=10)
Copy link
Member

Choose a reason for hiding this comment

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

I think color='gray' would be more explicit.

examples/linear_model/plot_ols_ridge_variance.py Outdated

clf.fit(X_train, y_train)
ax.plot(X_test, clf.predict(X_test), linewidth=2, color='blue')
ax.scatter(X_train, y_train, s=30, c='r', marker='+', zorder=10)
ax.scatter(X_train, y_train, s=30, color='r', marker='+', zorder=10)
Copy link
Member

Choose a reason for hiding this comment

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

I think color='red' would be more explicit.

@qinhanmin2014
Copy link
Member Author

@ogrisel If we use gray instead of 0.5, we can keep c. I guess this will make this PR easier to review.
ping @jnothman @amueller for the blocker.

@qinhanmin2014 qinhanmin2014 changed the title MNT Use color instead of c in ax.scatter MNT Use name instead of float to specify colors Sep 30, 2018
@qinhanmin2014 qinhanmin2014 mentioned this pull request Sep 30, 2018
Closed
@qinhanmin2014 qinhanmin2014 added this to the 0.20.1 milestone Sep 30, 2018
@ogrisel
Copy link
Member
ogrisel commented Oct 1, 2018

Let's merge. I still think that using color='gray' is more correct than c='gray' but fair enough.

@ogrisel ogrisel merged commit a3616f6 into scikit-learn:master Oct 1, 2018
@qinhanmin2014
Copy link
Member Author

Let's merge. I still think that using color='gray' is more correct than c='gray' but fair enough.

Thanks. I've opened #12224 to further improve the plot (including the title & axis) and this can serve as part of that issue, but AFAIK c is very similar to color in many cases, and I don't see why color is more correct.

@qinhanmin2014 qinhanmin2014 deleted the circleci-failure branch October 4, 2018 01:41
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Oct 15, 2018
@qinhanmin2014 @jnothman
MNT Use name instead of float to specify colors (scikit-learn#12199)
9136311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

Assignees
No one assigned
Labels
Blocker
Projects
None yet
Milestone
0.20.1
Development

Successfully merging this pull request may close these issues.
2 participants
@qinhanmin2014 @ogrisel
0