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

Finish issue 3804 and pass all the tests of number_line #3962

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

Conversation

liuyiheng0113
Copy link

@liuyiheng0113 liuyiheng0113 commented Oct 19, 2024

Overview: What does this pull request change?

Change number_line.py and add animatable_coordinate_systems.py

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

To fix issue #3804

Links to added or changed documentation pages

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

@@ -0,0 +1,114 @@
from __future__ import annotations

from manim import *

Check notice

Code scanning / CodeQL

'import *' may pollute namespace Note

Import pollutes the enclosing namespace, as the imported module
manim
does not define '__all__'.
class AnimatableNumberLine(NumberLine):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.x_range_tracker = ValueTracker(self.x_range)

Check warning

Code scanning / CodeQL

Overwriting attribute in super-class or sub-class Warning

Assignment overwrites attribute x_range_tracker, which was previously defined in superclass
NumberLine
.
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.

Thanks for trying to tackle this issue, it's something we've wanted to get to for a while now :)
However, I'm not sure that creating a new class is the right way to go, because it could cause confusion when to use the normal classes vs the Animatable* versions. I believe it should be possible to use the original classes to add this behavior.
What do you think?

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.

2 participants