Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix bug in Axes.bar_label(label_type='center') for non-linear scales. #23698

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

Merged
merged 3 commits into from
Sep 11, 2022

Conversation

stefmolin
Copy link
Contributor

@stefmolin stefmolin commented Aug 21, 2022

PR Summary

Fixes #23688

Incorporated @anntzer's solution from the issue into the Axes.bar_label() method.


Example using a log scale:

import matplotlib.pyplot as plt

fig, ax = plt.subplots()
bar_container = ax.barh(['c', 'b', 'a'], [1_000, 5_000, 7_000])
ax.set_xscale('log')
ax.set_xlim(1, None)
ax.bar_label(bar_container, label_type='center')

Current output:
Screen Shot 2022-08-20 at 6 07 03 PM

Output with this change:
Screen Shot 2022-08-21 at 7 08 47 PM

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@stefmolin
Copy link
Contributor Author

I'm not sure what would be the best way to test this. We could check that the label's xy is (0.5, 0.5), but that doesn't seem too robust; does this need an image test?

@tacaswell tacaswell added this to the v3.7.0 milestone Aug 26, 2022
@stefmolin stefmolin changed the title [WIP] Fix bug in Axes.bar_label(label_type='center') for non-linear scales. Fix bug in Axes.bar_label(label_type='center') for non-linear scales. Aug 28, 2022
@oscargus
Copy link
Member

Yes, I believe that an image test is good here.

@stefmolin
Copy link
Contributor Author

Yes, I believe that an image test is good here.

@oscargus - Can you walk me through how to do that?

@timhoffm
Copy link
Member

See https://matplotlib.org/devdocs/devel/testing.html#writing-an-image-comparison-test for a basic introduction. If you need more help, please ask.

@oscargus
Copy link
Member

Just in case (this probably won't work directly):

@image_comparison(["centered_bar_label_nonlinear.png"])
def test_centered_bar_label_nonlinear()
    fig, ax = plt.subplots()
    bar_container = ax.barh(['c', 'b', 'a'], [1_000, 5_000, 7_000])
    ax.set_xscale('log')
    ax.set_xlim(1, None)
    ax.bar_label(bar_container, label_type='center')
    ax.set_axis_off()

You will on the first local run get a failure and a file result_images/test_axes/centered_bar_label_nonlinear.png (if you put it in test_axes.py) which should be copied to baseline_images/test_axes and then everything should work.

I sometimes reduce the image size to get smaller figure sizes. I think that can probably be done here without losing anything.

@tacaswell
Copy link
Member

To add more opinions, an svg might make sense here (not sensitive to the freetype version issue but is sensitive to text placement changes).

@stefmolin
Copy link
Contributor Author

Thanks all! I'll look into it.

@timhoffm
Copy link
Member

timhoffm commented Sep 2, 2022

To add more options: If we don't want an image comparison (for size, font glitch or whatever reasons) IMHO it would already be sufficient to take the example from the original post and check that the texts are anchored on the left half of the figure (i.e. transform the coordinates to figure coordinates and check that x < 0.5).

Going one step further, one could also transform the bar limits to figure coordinates and then check that the text is centered between the bar limits in figure coordinates (which is the exact definition of what we expect).

That said, @stefmolin don't get overwhelmed by all our ideas 😄. Either of the image comparison using an svg or the logical coordinate test would be fine. Choose the one you are more comfortable with.

@stefmolin
Copy link
Contributor Author

I added an image test with a PNG file since it says it can't do SVG or PDF comparison tests on my system. Let me know if we still need an SVG test, and if so, how to generate the file in the first place since it just skips the test.

Screen Shot 2022-09-05 at 6 55 09 PM

@stefmolin
Copy link
Contributor Author

I added an image test with a PNG file since it says it can't do SVG or PDF comparison tests on my system. Let me know if we still need an SVG test, and if so, how to generate the file in the first place since it just skips the test.

Screen Shot 2022-09-05 at 6 55 09 PM

We ended up talking about this in the new contributor's meeting. I might be missing some dependencies for the image test. I will look into it and report back.

@timhoffm
Copy link
Member

timhoffm commented Sep 7, 2022

I might be missing some dependencies for the image test.

Might be Inkscape.

See also https://matplotlib.org/devdocs/devel/dependencies.html#dependencies-for-testing-matplotlib.

@stefmolin
Copy link
Contributor Author

It was Inkscape. I see there was some new logic added to make it easier to understand what is going on 😄

if extension not in comparable_formats():
reason = {
'pdf': 'because Ghostscript is not installed',
'eps': 'because Ghostscript is not installed',
'svg': 'because Inkscape is not installed',
}.get(extension, 'on this system')
pytest.skip(f"Cannot compare {extension} files {reason}")

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I'm glad you were able to get that to work, @stefmolin. This works great on window resizes too which is nice :)

@greglucas greglucas merged commit 7d0030c into matplotlib:main Sep 11, 2022
@stefmolin stefmolin deleted the bar-label-center branch September 11, 2022 01:22
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.

[Bug]: Axes.bar_label() on log scale does not center the label.
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.