Fixed bug in :class:~.NumberPlane with strictly positive or strictly negative values for x_range and y_range

[cuppajoeman]

Changelog / Overview

Motivation

NumberPlane doesn't work correctly when you have two positive or two negative numbers set as the (x/y)_range.

class LineGraphExampleBroken(Scene):
    def construct(self):

        pos_x_range = (0, 7)

        pos_y_range = (2, 6)

        x_vals = [0, 1.5, 2, 2.8, 4, 6.25]
        y_vals = [2, 5, 4.25, 6, 4.5, 2.75]

        plane = NumberPlane(
            x_range = pos_x_range,
            y_range = pos_y_range,
            #x_length = 7,
            axis_config={"include_numbers": True},
        )
        plane.center()

        #value_tests = [(x_vals, y_vals)]
        line_graph = plane.get_line_graph(
            x_values = x_vals,
            y_values = y_vals,
            line_color=GOLD_E,
            vertex_dot_style=dict(stroke_width=3,  fill_color=PURPLE),
            stroke_width = 4,
        )
        self.add(plane, line_graph)

generates an extra line:

The problem is here:

manim/manim/mobject/coordinate_systems.py

Lines 1594 to 1597 in 39d429d

	ranges = (
	np.arange(0, axis_perpendicular_to.x_max, step),
	np.arange(0, axis_perpendicular_to.x_min, -step),
	)

Where you can see that in the above example that that the arange is going from 0 to 6 when it should really only have 6-2 = 4 elements.

This type of situation only arises when both the x_min and x_max are either both positive or both negative, in my example they're both positive then we should only iterate up from 0 to 6-2=4. If they were both negative, say -6, -2 then we would iterate from -6-(-2) = -4 to 0.

If you look at my changes you can see that I have implemented the above logic more generally, and left the code the same for when they are on both sides of 0.

We can now see that this fixes the error, and I have tested it for all possible combinations:

class LineGraphExample(Scene):
    def construct(self):

        pos_x_range = (0, 7)
        neg_x_range = (-7, 0)

        pos_y_range = (2, 6)
        neg_y_range = (-6, -2)

        x_vals = [0, 1.5, 2, 2.8, 4, 6.25]
        y_vals = [2, 5, 4.25, 6, 4.5, 2.75]

        testing_data = [ (pos_x_range, pos_y_range, x_vals, y_vals), (pos_x_range, neg_y_range, x_vals, [-v for v in y_vals]), (neg_x_range, pos_y_range, [-v for v in x_vals], y_vals), (neg_x_range, neg_y_range, [-v for v in x_vals], [-v for v in y_vals])]

        for test_data in testing_data:

            x_range, y_range, x_vals, y_vals = test_data

            plane = NumberPlane(
                x_range = x_range,
                y_range = y_range,
                #x_length = 7,
                axis_config={"include_numbers": True},
            )
            plane.center()

            #value_tests = [(x_vals, y_vals)]
            line_graph = plane.get_line_graph(
                x_values = x_vals,
                y_values = y_vals,
                line_color=GOLD_E,
                vertex_dot_style=dict(stroke_width=3,  fill_color=PURPLE),
                stroke_width = 4,
            )
            self.play(FadeIn(VGroup(plane, line_graph)))
            self.play(FadeOut(VGroup(plane, line_graph)))

LineGraphExample.mp4

Explanation for Changes

Fixes a bug

Documentation Reference

Testing Status

Further Comments

Checklist

• I have read the Contributing Guidelines
• I have written a descriptive PR title (see top of PR template for examples)
• I have written a changelog entry for the PR or deem it unnecessary
• My new functions/classes either have a docstring or are private
• My new functions/classes have tests added and (optional) examples in the docs
• My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

• The PR title is descriptive enough
• The PR is labeled correctly
• The changelog entry is completed if necessary
• Newly added functions/classes either have a docstring or are private
• Newly added functions/classes have tests added and (optional) examples in the docs
• Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

[huguesdevimeux]

Thanks for the PR !
This needs a graphical unit test, given that it is a breaking bug.

I let a suggestion, please check it out !

[hydrobeam]

Thank you for handling this bug and for the descriptive code examples/explanation! The fix itself lgtm, but I left a suggestion to make the code simpler.

Also, a test should be added for this bug to ensure it doesn't pop up again. You can probably just use the examples in the PR description and count the number of lines produced.

[hydrobeam]

Ah, the checks are failing since I used x_range for the NumberLine. I gave NumberLine this attribute when working on #1812. Feel free to use x_min/x_max instead.

[hydrobeam]

Is this supposed to be commented? 🤔 What does wrong mean here

[cuppajoeman]

Hey hydrobeam, that was just when I was testing out how to set the attribute correctly. I have fixed it in the newest push.

[hydrobeam]

I fixed the broken text (and some bad typing) so this PR LGTM now.