-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fixing _get_standard_colors to honor num_colors arguments (#20585) #22439
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.
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
Conversation
Hello @atulagrwl! Thanks for updating the PR.
Comment last updated on August 21, 2018 at 10:57 Hours UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good at a glance.
Could you add a release note to 0.24.0 in the bugfix section?
pandas/tests/plotting/test_series.py
Outdated
@@ -268,6 +269,34 @@ def test_bar_user_colors(self): | |||
(1., 0., 0., 1.)] | |||
assert result == expected | |||
|
|||
def test_bar_user_colors_limited(self): | |||
s = Series([1, 2, 3, 4]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a reference to the original issue in a comment here.
@TomAugspurger - Thanks for quick review. Would really appreciate merging it to master soon (this would be my first contribution to get into master). |
@@ -1233,6 +1233,16 @@ def _plot(cls, ax, x, y, w, start=0, log=False, **kwds): | |||
def _start_base(self): | |||
return self.bottom | |||
|
|||
def _get_colors(self, num_colors=None, color_kwds='color'): | |||
color = self.kwds.get(color_kwds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a doc-string here
@@ -91,7 +91,9 @@ def _maybe_valid_colors(colors): | |||
# mpl will raise error any of them is invalid | |||
pass | |||
|
|||
if len(colors) != num_colors: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment here
tests are failing as well |
@atulagrwl do you plan on making the changes requested of you above? I'd love to see this bugfix get merged. |
Closing as stale. Ping if you'd like to pick back up |
git diff upstream/master -u -- "*.py" | flake8 --diff