Skip to content

Clean up code: Replace exceptions, remove unused parameters, and fix type annotations in Animation, ShowPartial, Create, and DrawBorderThenFill #4214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

irvanalhaq9
Copy link
Contributor

@irvanalhaq9 irvanalhaq9 commented Apr 7, 2025

Overview: What does this pull request change?

This pull request introduces some improvements:

  • Fix typo
  • Removes **kwargs from Animation.__init__, so it raises TypeError immediately when unknown arguments are passed.
  • Replaces NotImplementedError with a more appropriate TypeError in ShowPartial.
  • Removes unused parameters and attributes (draw_border_animation_config, fill_animation_config) from DrawBorderThenFill.
  • Updates the return type of _get_bounds in both ShowPartial and Create to better reflect actual usage.
  • Removes kwargs["final_alpha_value"] = 0 from TransformMatchingAbstractBase.
  • Adds _original__init__ = __init__ to the Animation base class to prevent AttributeError when calling Animation.set_default() with no arguments.

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

  1. Currently, Animation silently accepts unexpected keyword arguments without raising an error. This behavior may lead to confusion or silent bugs, since the user might think the argument is doing something, when in fact it is not used at all. For example:

    Create(Square(), rate_functions=linear)

    This passes silently, even though rate_functions is not a valid argument.

    NOTE
    For some animation classes, such as Create, FadeIn, Rotate, and Transform, which define their own parameters, but rely on default values from the parent class for some parameters (by passing **kwargs to super().__init__()), auto-completion cannot work properly.

    For example, if you want to use the rate_func parameter and start typing rate, the auto-completion may suggest rate_functions, which is not a parameter, but a globally imported module in Manim. Users might mistakenly think that it's a valid parameter name, but the code won’t work as intended — and the problem is, Manim does not throw an error for that.

    In contrast, Mobject raises a TypeError on unknown arguments:

    Square(say="Hello")
    # TypeError: Mobject.__init__() got an unexpected keyword argument 'say'

    So, this PR makes it raise an exception (TypeError) immediately when unknown arguments are passed. In addition, it adds use_override to the __init__ to avoid TypeError when passed from subclass's constructor.

  2. Replace NotImplementedError with TypeError in ShowPartial

    • Previously, passing a non-VMobject raised a NotImplementedError, which was misleading since it’s clearly a type check.
    • Now it raises a more appropriate TypeError, matching the docstring:
      raise TypeError(f"{self.__class__.__name__} only works for VMobjects.")
    • The class docstring explicitly states:
      Raises
      ------
      TypeError
          If `mobject` is not an instance of `~.VMobject`.
      
  3. Remove unused parameters and attributes from DrawBorderThenFill

    • draw_border_animation_config and fill_animation_config are not used at all—they are just stored but never actually do anything. I double-checked with git grep and couldn't find any usage elsewhere. They're also unused in manimgl. So in this PR, I suggest removing them to keep things cleaner and less confusing.

    • Verified with:

      git grep output
      $ git grep draw_border_animation_config
      manim/animation/creation.py:        draw_border_animation_config: dict = {},  # what does this dict accept?
      manim/animation/creation.py:        self.draw_border_animation_config = draw_border_animation_config
      
      $ git grep fill_animation_config
      manim/animation/creation.py:        fill_animation_config: dict = {},
      manim/animation/creation.py:        self.fill_animation_config = fill_animation_config
  4. Update _get_bounds return type annotation

    • Changed from -> None and -> tuple[int, float] to -> tuple[float, float] in ShowPartial and Create.
    • The return value is used in pointwise_become_partial, which expects two floats.
  5. Remove redundant kwargs["final_alpha_value"] = 0 in TransformMatchingAbstractBase
    final_alpha_value is never read or used anywhere in the codebase (verified via git grep), and there is no documentation or comment about it. Removing it simplifies the code without affecting functionality.

    angle=TAU is also removed because in Rotating the parameter is radians, not angle, and the default value is TAU, so no problem removing it.

    Removing them is needed because from this PR, they are considered unexpected keyword argument, and TypeError is raised.

  6. The set_default() method uses cls._original__init__ to restore the original __init__ method when no keyword arguments are passed. However, _original__init__ is not defined in the Animation base class, which causes an error when set_default() is called directly on Animation. This fix ensures that the _original__init__ is always available, even in the base class.

Links to added or changed documentation pages

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

@irvanalhaq9 irvanalhaq9 changed the title Clean up ShowPartial type check and remove unused parameters and attributes in DrawBorderThenFill and fix some return types. Clean up Animation, ShowPartial, DrawBorderThenFill and Create Apr 7, 2025
@irvanalhaq9 irvanalhaq9 changed the title Clean up Animation, ShowPartial, DrawBorderThenFill and Create Clean up Animation, ShowPartial, Create, and DrawBorderThenFill Apr 7, 2025
@irvanalhaq9 irvanalhaq9 changed the title Clean up Animation, ShowPartial, Create, and DrawBorderThenFill Clean up ShowPartial, Create, and DrawBorderThenFill Apr 7, 2025
@irvanalhaq9 irvanalhaq9 changed the title Clean up ShowPartial, Create, and DrawBorderThenFill Clean up code: Replace exceptions, remove unused parameters, and fix type annotations ShowPartial, Create, and DrawBorderThenFill Apr 8, 2025
@irvanalhaq9 irvanalhaq9 marked this pull request as draft April 9, 2025 09:06
@irvanalhaq9 irvanalhaq9 marked this pull request as ready for review April 9, 2025 10:12
@irvanalhaq9 irvanalhaq9 changed the title Clean up code: Replace exceptions, remove unused parameters, and fix type annotations ShowPartial, Create, and DrawBorderThenFill Clean up code: Replace exceptions, remove unused parameters, and fix type annotations in Animation, ShowPartial, Create, and DrawBorderThenFill Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

1 participant