Skip to content
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

Improve Updater signature checks for substantial speedups, add UpdaterWrappers and more Updater aliases #3807

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

Conversation

chopan050
Copy link
Contributor

@chopan050 chopan050 commented Jun 16, 2024

Closes #321

General description

In the issue described, Leo mentions that calling inspect.signature() every time we need to compute whether an Updater is time-based or not, takes a considerable amount of time when there are a lot of updaters in a Scene.

Leo proposed that every updater must always have two parameters and made a PR about it, but it was rejected.

Given an updater func, my first attempt was accessing func.__code__.co_varnames (local variable names in the function code) and slice it from 0 to func.__code__.co_argcount (number of positional parameters) to get the names of positional parameters.

However, as JasonGrace commented his concerns about more advanced use cases, I needed something more.

Therefore, in this PR I introduce a MobjectUpdaterWrapper, which takes an updater function, saves it in its .updater attribute, and uses inspect.signature() on it to calculate whether it's time-based or not. It stores the result in its .is_time_based, and this computation only happens once (when instantiating MobjectUpdaterWrapper), rather than every time the updater is called, leading to a substantial speedup when updating Mobjects.

In MobjectUpdaterWrapper I also address the concerns raised by JasonGrace, Leo and myself:

  • What if the updater function has parameters with default values?
  • What if there's no parameter named dt in the signature?
  • What if the function only has two parameters, but the 2nd one has a default value and is not intended to receive a dt value?
  • What if the function is a method and its 1st parameter is self?
  • What if the function isn't a method, but the 1st parameter is self anyways?
  • etc.

which is why I decided to make a separate class in the first place: the logic which attempts to address all these concerns is very long and shouldn't bloat the Mobject.add_updater() code. Plus, I wanted to replicate the same for OpenGLMobject as well.

Because I wanted to make the same changes in OpenGLMobject, the best course of action was taking all this code into a different module, specifically manim.utils.updaters.

I also noticed that the relatively obscure classes Object3D and its subclass Mesh also have updaters which work in a very similar fashion, so I added the MeshUpdater type alias and the MeshUpdaterWrapper class as well.

Finally, Scene also has updaters, but very different ones (they always accept a dt: float and return None), so I also added a SceneUpdater type alias.

Profiling

An example scene with many updaters:

class MovingSquares(Scene):
    def construct(self):
        t = ValueTracker()
        for y in range(4, -5, -2):
            for x in range(-8, 9, 2):
                updater = (lambda x, y: (
                    lambda square, dt: square.move_to([
                        x + np.cos(t.get_value()),
                        y + (1 if x % 4 == 0 else -1) * np.sin(t.get_value()),
                        0,
                    ])
                ))(x, y)
                self.add(Square(1).add_updater(updater))

        self.play(t.animate.set_value(2*TAU), run_time=10, rate_func=linear)

Notice how the inspect.signature() block completely disappears from the Mobject.update() time:

Before After
image image

Links to added or changed documentation pages

https://manimce--3807.org.readthedocs.build/en/3807/reference/manim.utils.updaters.html

Further Information and Comments

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

manim/scene/scene.py Fixed Show fixed Hide fixed
@chopan050 chopan050 added refactor Refactor or redesign of existing code maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements performance typehints For adding/discussing typehints labels Jun 16, 2024
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

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

Haven't looked through the whole thing, but just off a glance through, I left some comments

Hopefully, this makes the updater situation better in future versions of manim :)

manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
manim/utils/updaters.py Outdated Show resolved Hide resolved
@JasonGrace2282 JasonGrace2282 added this to the v0.19.0 milestone Jun 21, 2024
@JasonGrace2282 JasonGrace2282 removed maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements typehints For adding/discussing typehints labels Jul 17, 2024
@JasonGrace2282 JasonGrace2282 modified the milestones: v0.19.0, v0.20.0 Jul 23, 2024
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance refactor Refactor or redesign of existing code
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Deprecate different signatures for updaters
2 participants