Skip to content
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

FIX: allow single-string color for scatter #12431

Merged
merged 3 commits into from Oct 15, 2018
Merged

FIX: allow single-string color for scatter #12431

merged 3 commits into from Oct 15, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Oct 7, 2018 •
edited

PR Summary

Closes #12429
Closes #12438

New test fails master, passes this PR, and looks correct:

import matplotlib.pyplot as plt
import numpy as np

fig, ax = plt.subplots()
ax.scatter(np.arange(10), np.arange(10), c='0.5')
plt.show()

scatter

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak added this to the v3.0.x milestone Oct 7, 2018
@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 7, 2018
@jklymak jklymak requested a review from afvincent October 7, 2018 17:13
timhoffm
timhoffm previously approved these changes Oct 7, 2018
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Subject to CI passing.

tacaswell
tacaswell previously approved these changes Oct 7, 2018
View reviewed changes
@jklymak
Copy link
Member Author

jklymak commented Oct 7, 2018

Travis error is MacOS image tolerance of 0.005 changing for test_constrained_layout.py::test_colorbar_location. Not sure what changed on macOS to cause this, but it doesn't affect this PR.

@jklymak jklymak mentioned this pull request Oct 7, 2018
Closed
@ImportanceOfBeingErnest
Copy link
Member

I wonder if this is the correct fix. The documentation also states

c : color, sequence, or sequence of color, optional, default: 'b'
The marker color. Possible values:

  • A single color format string.
  • A sequence of color specifications of length n.

So for

plt.scatter([0,1],[2,3],c=("0.5", "0.7"))

one would expect two points with different shades of grey. But this is converted to an array of values and shown with the viridis colormap.
Not sure if the documentation is wrong here or the code.

@jklymak
Copy link
Member Author

jklymak commented Oct 8, 2018

Well, that didn't work for v2.2.3 either. I don't see any reason that the above shouldn't work the way you say, but its not a regression like #12429 is.

@jklymak jklymak mentioned this pull request Oct 8, 2018
Closed
@jklymak
Copy link
Member Author

jklymak commented Oct 8, 2018

The newest two commits allow lists like ['0.5', '0.7', '0.9', '0.5'] which I agree the docs say we should be able to do.

QuLogic
QuLogic previously requested changes Oct 8, 2018
View reviewed changes
setup.py Outdated
@@ -15,13 +15,12 @@
error = """
Matplotlib 3.0+ does not support Python 2.x, 3.0, 3.1, 3.2, 3.3, or 3.4.
Beginning with Matplotlib 3.0, Python 3.5 and above is required.

Copy link
Member

Choose a reason for hiding this comment

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

You seem to have some extra changes in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sorry - will clean up

@jklymak jklymak force-pushed the fix-single-string-color-scatter branch 5 times, most recently from 976ae9f to 7e58b82 Compare October 8, 2018 15:48
@jklymak jklymak dismissed stale reviews from tacaswell and timhoffm October 8, 2018 16:40

Added more to the PR (list of numeric strings)

timhoffm
timhoffm reviewed Oct 8, 2018
View reviewed changes
lib/matplotlib/tests/test_axes.py
@@ -1763,6 +1765,9 @@ def test_scatter_color(self):
([0.5]*3, None), # should emit a warning for user's eyes though
([0.5]*4, None), # NB: no warning as matching size allows mapping
([0.5]*5, "shape"),
# list of strings:
(['0.5', '0.4', '0.6', '0.7'], None),
(['0.5', 'red', '0.6', 'C5'], None),
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add mixtures of string and float to the test? I.e. c=[0.5, '0.5'] and c=['0.5', 0.5].

Not sure what this should result in, but it should either be a defined output or raise an appropriate error. Either way it should be tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. Done!

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that it errors, which I think is correct.

@jklymak jklymak force-pushed the fix-single-string-color-scatter branch from 7e58b82 to 3cd9b69 Compare October 8, 2018 17:37
@jklymak
Copy link
Member Author

jklymak commented Oct 13, 2018

ping @QuLogic to clear block (setup.py was inadvertently modified)

@QuLogic QuLogic dismissed their stale review October 14, 2018 00:06

Extraneous changes removed.

@tacaswell tacaswell merged commit ab86e8f into matplotlib:master Oct 15, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 15, 2018
@tacaswell
Backport PR matplotlib#12431: FIX: allow single-string color for scatter
1f2ca01
@meeseeksmachine meeseeksmachine mentioned this pull request Oct 15, 2018
Merged
jklymak added a commit that referenced this pull request Oct 15, 2018
@jklymak
Merge pull request #12531 from meeseeksmachine/auto-backport-of-pr-12…
e5a6cd6 
…431-on-v3.0.x

Backport PR #12431 on branch v3.0.x (FIX: allow single-string color for scatter)
@jklymak jklymak deleted the fix-single-string-color-scatter branch October 22, 2018 17:41
@ImportanceOfBeingErnest ImportanceOfBeingErnest mentioned this pull request Oct 29, 2018
Closed
This was referenced Nov 1, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@tacaswell tacaswell tacaswell left review comments

@QuLogic QuLogic QuLogic left review comments

@afvincent afvincent Awaiting requested review from afvincent

Assignees
No one assigned
Labels
API: argument checking Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.0.1
Development

Successfully merging this pull request may close these issues.

Scatter doesn't accept a list of strings as color spec. scatter() does not accept gray strings anymore
5 participants
@jklymak @ImportanceOfBeingErnest @tacaswell @QuLogic @timhoffm