Fix crash due to bad rounding in BarChart when no y_step was provided and the max y value was close to 0

zeFresk commented 2 months ago

Overview: What does this pull request change?

This PR fixes a bug in BarChart that lead to a crash if the user did not provide the y_step (i.e. tick rate, the user provided y_range=[y_min, y_max]) and the y values were very close to 0 (round(max(ys), 2) == 0).

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

When the user only provided the minimum and maximum of the y_range, the code for BarChart was deducing the tick rate for the y axis using the following code:

round(max(self.values) / y_length, 2)

However, this returns 0 if the maximum is small, because it is rounded down to zero. This lead to a division by zero in a np.arrange downstream. This issue can be fixed by replacing the 2 by a computed value.

This PR replaces the hard-coded 2 with $\max\left(\lceil - \log_{10}(x) \rceil, ~ 2\right)$, with $x$ being equal to max(self.values) / y_length (unrounded y_step in the original code). To ensure retro-compatibility, the newly computed value can't go below 2, in the case of large y values.

Reviewer Checklist

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

JasonGrace2282 commented 2 months ago

Hey thanks for the PR! Do you mind adding a (non-graphical) test for this?

zeFresk commented 1 month ago

Hello and thank you for the reply. I added one simple test and checked to make sure it was failing before this PR, but don't throw errors after this PR.

If it's not enough, I could add more tests.

JasonGrace2282 commented 1 month ago

I think it looks fine from my cursory testing :) Could you please just fix the pre-commit linting errors, and then explain where you got your formula from (preferably as a comment in the code)?

zeFresk commented 1 month ago

I added comments and fixed the linting. Do you think the explanations are clear enough? I can expand them if needed.

zeFresk commented 1 month ago

I may have broken something with my last commits, because I recently made an animation with very low values that worked perfectly from 6 bars of values 1/10000th to 1. I think forcing the y_step could be a good solution too. In fact I did that at first, then thought that the code I used may be useful as it fixed the problem and provided smooth transitions when animating.

Could you share some non-working examples, please? I could also try to understand what's going wrong and fix it.

JasonGrace2282 commented 1 month ago

Could you share some non-working examples, please? I could also try to understand what's going wrong and fix it.

from manim import *

class BarChartExample(Scene):
    def construct(self):
        values = [1 / 10000 for _ in range(8)]
        chart = BarChart(
            values=values,
            bar_names=["one", "two", "three", "four", "five"],
            y_range=[0, 2 / 10000],
            y_length=6,
            x_length=10,
            x_axis_config={"font_size": 36},
        )

        c_bar_lbls = chart.get_bar_labels(font_size=48)

        self.add(chart, c_bar_lbls)

config.preview = True
BarChartExample().render()

Is what I tried, which results in

ManimCommunity / manim