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.

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

Add functionality to plot bar and barh with string labels (Implement #2516) #4266

Merged
merged 1 commit into from Apr 12, 2015
Merged

Add functionality to plot bar and barh with string labels (Implement #2516) #4266

merged 1 commit into from Apr 12, 2015

Conversation

umairidris
Copy link

This patch is an attempt to add the functionality to pass in strings to label the axis of a bar graph (issue #2516). The patch shouldn't break any existing API. I also center the labels based on the width/height passed for a cleaner more professional look (please view the tests). I am happy to hear any feedback and/or concerns!

Cheers!
Umair

@jenshnielsen
Copy link
Member

The Travis docs build failure is not an issue with this Pull request. Fixed by #4267

@@ -1960,6 +1965,11 @@ def make_iterable(x):
left *= nbars
if len(height) == 1:
height *= nbars
if nbars and cbook.is_sequence_of_strings(bottom):
ticks_loc = [i + height[i]/2 for i in range(nbars)]
Copy link
Member

Choose a reason for hiding this comment

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

I would do this as [i + h / 2 for i, h in enumerate(height)], same for the logic in bar

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, this looks a lot better now! Fixed via 1627e8e

@tacaswell
Copy link
Member

👍 for the idea, I am a bit wary of overloading bottom and left in this way as now you can either label them or have non-zero baseline, but not both.

@umairidris
Copy link
Author

👍 for the idea, I am a bit wary of overloading bottom and left in this way as now you can either label them or have non-zero baseline, but not both.

You're right. To address this I added a check and raised ValueError if bottom (bar) or left (barh) are a list of strings. Perhaps we should more explicitly state this in the docstring as well. If you have any suggestions on how to improve this let me know!

@tacaswell
Copy link
Member

Sorry, I transposed the issue. Now you can't specify both the labels and non-unit bar spacing which is much worse.

@tacaswell tacaswell added this to the next point release milestone Mar 23, 2015
@umairidris
Copy link
Author

Ah, I see what you mean. Perhaps a better way to do this is to not touch left and bottom and instead add an extra kwarg 'labels' or something similar?

The use case I am thinking about is: bar([1,2,3], [1,1,1], labels=['a', 'b', 'c'])

@tacaswell
Copy link
Member

Yes, I think that is a better route. I would also not move the ticks relative to the bar unless explicitly asked to. Both bar* functions have an align kwarg which should still be respected.

@tacaswell
Copy link
Member

@umairidris You should also have a look at how boxplot handles this sort of thing. iirc bxp has a bunch of tick-management logic at the bottom of it.

@umairidris
Copy link
Author

Thanks for the feedback and advice @tacaswell! I have addressed it via commit c04bf79.

A few points I should mention:

  • I wanted to match the 'label' bxpstats key used by bxp. However, label is a kwarg that is already being used by bar. I am not sure what the best name for my added kwarg should be... tick_label (currently used)? bar_label? text_label?
  • I added the ability to pass in the labels as a single string to remain consistent with the rest of the API. You can now do bar(0, 1, tick_label='a') (please see added test).
  • I moved the code to set the labels to the bottom of the bar function. This mimics bxp and also fixes interfering with autoscale (self.autoscale_view() needs to be called before the tick labels are set).
  • As requested, bar* align kwarg is now respected (see updated tests).

I hope I addressed your concerns. Please let me know if you have any further suggestions!

@tacaswell
Copy link
Member

This looks good, just one code comment.

Can you add an entry in https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new so that this gets advertised?

@@ -1926,6 +1930,8 @@ def make_iterable(x):
width = make_iterable(width)
_bottom = bottom
bottom = make_iterable(bottom)
_tick_label = tick_label
Copy link
Member

Choose a reason for hiding this comment

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

I would do this as

label_ticks_flag = not tick_label is None

Which I think is a bit clearer.

Copy link
Member

Choose a reason for hiding this comment

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

tick_label is not None, please
On Mar 29, 2015 11:33 PM, "Thomas A Caswell" notifications@github.com
wrote:

In lib/matplotlib/axes/_axes.py
#4266 (comment):

@@ -1926,6 +1930,8 @@ def make_iterable(x):
width = make_iterable(width)
_bottom = bottom
bottom = make_iterable(bottom)

  •    _tick_label = tick_label
    

I would do this as

label_ticks_flag = not tick_label is None

Which I think is a bit clearer.


Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4266/files#r27365250.

Copy link
Member

Choose a reason for hiding this comment

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

erm, yes, what @WeatherGod said

@tacaswell
Copy link
Member

@umairidris This needs to be rebased, not sure why.

@umairidris
Copy link
Author

@tacaswell Done! My branch had the old docstrings (not numpy format) which was causing merge conflicts. All fixed now. :)

Umair I.

@tacaswell
Copy link
Member

Great. I will merge this as soon as travis passes again.

tacaswell added a commit that referenced this pull request Apr 12, 2015
ENH : pass list of strings to label bar/barh plots

Add functionality to plot bar and barh to label bars with list strings (closes #2516)
@tacaswell tacaswell merged commit a6af675 into matplotlib:master Apr 12, 2015
@umairidris umairidris deleted the bar_labels branch April 12, 2015 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants