Skip to content

FIX: allow single-string color for scatter#12431

Merged
tacaswell merged 3 commits intomatplotlib:masterfrom
jklymak:fix-single-string-color-scatter
Oct 15, 2018
Merged

FIX: allow single-string color for scatter#12431
tacaswell merged 3 commits intomatplotlib:masterfrom
jklymak:fix-single-string-color-scatter

Conversation

@jklymak
Copy link
Copy Markdown
Member

@jklymak jklymak commented Oct 7, 2018 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

ImportanceOfBeingErnest commented Oct 7, 2018

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
Copy Markdown
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
Copy Markdown
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
Comment thread setup.py
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
Copy Markdown
Member

@QuLogic QuLogic Oct 8, 2018

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
Copy Markdown
Member Author

@jklymak jklymak Oct 8, 2018

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
Comment thread lib/matplotlib/tests/test_axes.py
([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
Copy Markdown
Member

@timhoffm timhoffm Oct 8, 2018

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
Copy Markdown
Member Author

@jklymak jklymak Oct 8, 2018

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
Copy Markdown
Member Author

@jklymak jklymak Oct 8, 2018

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
Copy Markdown
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