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

bugfix: BarGraph drawing numbers on the y axis even when "y_axis.include_numbers" is False #3906

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Tmodrzyk
Copy link

@Tmodrzyk Tmodrzyk commented Aug 14, 2024

Overview: What does this pull request change?

Numbers were always drawn without checking the value of y_axis.include_numbers.
Numbers are now drawn on the y_axis only if y_axis.include_numbers is True.

Motivation and Explanation: Why and how do your changes improve the library?

I think this is the expected behavior, otherwise the user can't hide these numbers.

Links to added or changed documentation pages

No changes in documentation required.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

- Numbers are now drawn on the `y_axis` only if `y_axis.include_numbers` is True
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this bug! Indeed, because Axes by default don't include numbers, it makes sense for BarChart, a subclass of Axes, to not have numbers by default as well.

I left a comment on your change.

Apart from that comment, I would need you to please review the tests/test_graphical_units/test_probability.py. The graphical unit tests defined there are failing, because BarChart no longer contains numbers by default.

  • It makes sense for most of them to have numbers in the Y axis, so maybe you could pass y_axis_config={"include_numbers": True} as an extra argument to them.
  • The first test, test_default_chart, is more problematic, because now a "default chart" has no numbers. IMO, the best approach would be to replace it with two tests, test_default_version_chart and test_numbered_version_chart, where the latter has numbers and the former doesn't. However, you would have to regenerate the graphical unit tests for that to work: see https://docs.manim.community/en/stable/contributing/testing.html#graphical-unit-test

manim/mobject/graphing/probability.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants