Skip to content

Conversation

@hydrobeam
Copy link
Member

Changelog / Overview

Fix odd visuals and unnecessary functionality in vector_coordinate_label and move it to geometry.py as a method under Vector. Also includes compatibility for vector_space_scene

Motivation

BackgroundRectangle was raising some errors for vector_coordinate_label when animated with self.play(), so I removed its usages from this method since I couldn't find a reason for why it was there. Also, I removed the add_background_rectangles_to_entries=True from the method since it introduced some very odd looking labels.

Explanation for Changes

Usually, a label for a vector would be called by vector_coordinate_label and through passing a Vector mobject as the first argument (technically it could have been any Line). This makes the method explicitly attached to Vector and removes some useless functionality which causes errors and visual mishaps.

Old:

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

image

New:

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

image

Testing Status

Further Comments

The reason for the bug actually lies in BackgroundRectangle. This PR is simply intended to patch a rather useful method.

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

@hydrobeam hydrobeam changed the title Fix bug in :meth:vector_coordinate_label and move it to :class:~.geometry.py Fix bug in :meth:vector_coordinate_label and move it to :class:~.geometry.py Apr 27, 2021
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me, thanks for improving the implementation! I have one suggestion, please have a look and ping me once you have applied it (or want to discuss it).

I think that this PR fixes issues that the old implementation had, but at the same time it would be nice to have the option to have a background rectangle added -- maybe via an additional keyword parameter. It would also be a good first issue, could you simply open an issue for it and reference this PR?

Comment on lines 1296 to 1306
def coordinate_label(self, integer_labels=True, n_dim=2, color=WHITE):
"""A label based on the coordinates of the vector.
Parameters
----------
integer_labels : :class:`bool`, optional
Whether or not to round the coordinates to integers.
n_dim : `class`:`int`, optional
The number of dimensions of the vector.
color : :class:`~.Colors`, optional
The color of the label.
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather have the types as actual type hints. Also, I'm pretty sure that WHITE is just a str.

Suggested change
def coordinate_label(self, integer_labels=True, n_dim=2, color=WHITE):
"""A label based on the coordinates of the vector.
Parameters
----------
integer_labels : :class:`bool`, optional
Whether or not to round the coordinates to integers.
n_dim : `class`:`int`, optional
The number of dimensions of the vector.
color : :class:`~.Colors`, optional
The color of the label.
def coordinate_label(self, integer_labels: bool = True, n_dim : int = 2, color: str = WHITE):
"""A label based on the coordinates of the vector.
Parameters
----------
integer_labels
Whether or not to round the coordinates to integers.
n_dim
The number of dimensions of the vector.
color
The color of the label.

@behackl behackl added enhancement Additions and improvements in general refactor Refactor or redesign of existing code labels Apr 29, 2021
@hydrobeam
Copy link
Member Author

I've implemented your suggestions, so it should be good to go! @behackl

@behackl behackl merged commit ac17f75 into ManimCommunity:master Apr 29, 2021
@behackl behackl added this to the Release v0.6.0 milestone Apr 29, 2021
@jsonvillanueva jsonvillanueva added maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements and removed refactor Refactor or redesign of existing code labels Apr 30, 2021
@hydrobeam hydrobeam changed the title Fix bug in :meth:vector_coordinate_label and move it to :class:~.geometry.py Fix bug in :meth:vector_coordinate_label and move it to :class:geometry.py May 1, 2021
@hydrobeam hydrobeam changed the title Fix bug in :meth:vector_coordinate_label and move it to :class:geometry.py Fix bug and rename :meth:vector_coordinate_label to :meth:~.Vector.coordinate_label and move it to :class:geometry.py May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Additions and improvements in general maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants