enhance: Add background rectangle in Vector.coordinate_label based on keyword arg

[Rudransh24]

Overview: What does this pull request change?

As mentioned in #1423 this PR adds the option of displaying background rectangle in the coordinate_label function in the Vector class. This is done by adding a parameter in the function call.

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

In some discussions and PR #1407 it was mentioned how this small enhancement is required in order to give the user an option to be able to display background rectangle or not.

Links to added or changed documentation pages

Earlier, when Vector.coordinate_label was called, it did not have a parameter for showing background color of the coordinate labels. I have added a parameter to the coordinate_label function which enables user to provide a background color to the coordinates

Old:

class A(Scene):
    def construct(self):
        vect = Vector([1, 2])
        label = vect.coordinate_label()
        self.add(vect, label)

New:

class VectorCoordinateLabel(Scene):
    def construct(self):
        plane = NumberPlane()

        vect_1 = Vector([1, 2])
        vect_2 = Vector([-3, -2])
        label_1 = vect_1.coordinate_label(show_bg_rec=True)
        label_2 = vect_2.coordinate_label(color=YELLOW)

        self.add(plane, vect_1, vect_2, label_1, label_2)

Further Information and Comments

This is my first PR in an open source project (outside of Hacktober Fest). Open to suggestions and modifications.

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

[icedcoffeeee]

I glossed over your PR and left some comments

[icedcoffeeee]

This part is redundant no? Passing add_background_rectangles_to_entries in Matrix already does these.
Cancel that, why don't you pass include_background_rectangle to Matrix instead of add_background_rectangles_to_entries and adding a BackgroundRectangle separately?

[Rudransh24]

Oh yes, that sounds better actually. Thanks!
I have made the change in the function.

[icedcoffeeee]

The example for the method isn't showing up, as shown here. Could you fix that? The indentation of the manim code block isn't quite right.

[Rudransh24]

I didn't quite get this one. I saw the page and yes the example is missing. But upon checking the indentation in the function docstring and committing, I couldn't pinpoint the exact issue that could be causing the missing example. I tried crosschecking with the other examples present in the script as well. Will try investigating a bit more.

[icedcoffeeee]

Ah sorry, I misjudged, the problem was at the .. manim keyword at the start of the example, it needs a double colon

[Rudransh24]

Oh yes, found it. There was some build issue too due to wrong variable name in the example. Fixed both. Am able to see the example in the link now.

[huguesdevimeux]

thanks for the PR !

Could you :

• Put images examples of what your changes do,
• Add tests.

Thanks !

[Rudransh24]

thanks for the PR !

Could you :

• Put images examples of what your changes do,
• Add tests.

Thanks !

Hello @huguesdevimeux
I have added example and tests. Could you please review it?
Thanks!

[icedcoffeeee]

I don't think that colored background rectangles is a desired result.

at line 1623, you can make small change:

if show_bg_rect:
    label[1:].set_color(color)
else:
    label.set_color(color)

[Rudransh24]

I don't think that colored background rectangles is a desired result.

at line 1623, you can make small change:

if show_bg_rec:
    label[1:].set_color(color)
else:
    label.set_color(color)

Hello @icedcoffeeee
I made the change but I am still little confused what this is achieving as this does not change anything. Do we only need background color on the digits(which I did in the first commit) or the whole coordinate(which I am doing right now)?

[icedcoffeeee]

which I did in the first commit

Technically, your first commit did bring the desired result. My suggestion of changes was just a mere simplification 😅

[Rudransh24]

which I did in the first commit

Technically, your first commit did bring the desired result. My suggestion of changes was just a mere simplification 😅

Cancel that, why don't you pass include_background_rectangle to Matrix instead of add_background_rectangles_to_entries and adding a BackgroundRectangle separately?

After some observation I found out that there wasn't any need of BackgroundRectangle Function and a separate if-else condition, I think only passing a parameter show_bg_rec = True and add_background_rectangles_to_entries = show_bg_rec would give us the desired output which is displaying background on the entries of the coordinates.

label = Matrix(vect, add_background_rectangles_to_entries=show_bg_rec)
This line gives me the desired output!
Any thoughts?

[icedcoffeeee]

label = Matrix(vect, add_background_rectangles_to_entries=show_bg_rec)
This line gives me the desired output!

I don't think so, it still gives a colored background rect under each element.

Plus, personally when i pass show_bg_rec, i'd expect a background rect of the whole matrix rather than just the entries. But it might be different for everyone, we might need another's opinion.

[Rudransh24]

label = Matrix(vect, add_background_rectangles_to_entries=show_bg_rec)
This line gives me the desired output!

I don't think so, it still gives a colored background rect under each element.

Plus, personally when i pass show_bg_rec, i'd expect a background rect of the whole matrix rather than just the entries. But it might be different for everyone, we might need another's opinion.

Ok, so we actually need 2 different parameters one for the entries' background only, and one for the whole matrix.

[Rudransh24]

@huguesdevimeux @icedcoffeeee @behackl @hydrobeam any opinions?

[icedcoffeeee]

Hi sorry for the delay. I think a good method to customize the matrix would be to let users pass any keyword args as **kwargs instead. The coloring issue is best fixed from within the Matrix constructor. Currently, there isn't a way to specify a color to the matrix init. Could you make that possible?

[behackl]

Superseded by #2138.