Skip to content

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

Open
Tmodrzyk wants to merge 4 commits into
ManimCommunity:mainfrom
Tmodrzyk:main
Open

bugfix: BarGraph drawing numbers on the y axis even when "y_axis.include_numbers" is False#3906
Tmodrzyk wants to merge 4 commits into
ManimCommunity:mainfrom
Tmodrzyk:main

Conversation

@Tmodrzyk

@Tmodrzyk Tmodrzyk commented Aug 14, 2024

Copy link
Copy Markdown

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

@chopan050 chopan050 left a comment

Copy link
Copy Markdown
Member

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

Comment thread manim/mobject/graphing/probability.py Outdated
Tmodrzyk and others added 3 commits October 28, 2024 10:03
Co-authored-by: Francisco Manríquez Novoa <49853152+chopan050@users.noreply.github.com>
@Tmodrzyk

Copy link
Copy Markdown
Author

Hi @chopan050 I'm so sorry I completely forgot about that, thanks for the review.
I'm very busy at the moment but would be pleased to come back to it later (probably in August).

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