-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add type annotations for most of camera
and mobject.graphing
#4125
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
Add type annotations for most of camera
and mobject.graphing
#4125
Conversation
updates: - [github.com/astral-sh/ruff-pre-commit: v0.9.1 → v0.9.2](astral-sh/ruff-pre-commit@v0.9.1...v0.9.2)
6b8f27d
to
f767e8a
Compare
self.display_funcs: dict[ | ||
type[Mobject], Callable[[list[Mobject], PixelArray], Any] | ||
] = { | ||
VMobject: self.display_multiple_vectorized_mobjects, # type: ignore[dict-item] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up using a type ignore statement here.
assert isinstance(label_mob, VMobject) | ||
label_mobs.add(label_mob) | ||
parts.braces = braces | ||
parts.labels = label_mobs | ||
parts.label_kwargs = { | ||
parts.braces = braces # type: ignore[attr-defined] | ||
parts.labels = label_mobs # type: ignore[attr-defined] | ||
parts.label_kwargs = { # type: ignore[attr-defined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I chose to use an assert statement and a few type ignores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some suggestions I missed. Hopefully these will be the last 🙏🏼
def __init__(self, function, x_range=None, color=YELLOW, **kwargs): | ||
def __init__( | ||
self, | ||
function: Callable[[float], Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably fine for now that function
returns Any
instead of float
for compatibility with the parent class' function
attribute. In a subsequent PR, there should be a rewrite such that FunctionGraph.function
is different from ParametricFunction.function
.
tick_range, | ||
unit_decimal_places=decimal_number_config["num_decimal_places"], | ||
) | ||
|
||
self.add_labels( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange that this wasn't autocollapsed by pre-commit into a single line like this:
self.add_labels(dict(zip(tick_range, custom_labels)))
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
Co-authored-by: Francisco Manríquez Novoa <[email protected]>
# TODO: I think the method should be able to return more than just a single point. | ||
# E.g. see the implementation of it on line 2065. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this function (or rather its descendants) should be able to return more than a single point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually correct. The Axes.coords_to_points()
method I optimized in #3286 also uses that behavior. Thanks for reminding me of it.
This behavior is probably too complex to type for now, so let's leave the TODO there.
I took another look at the code and found a number of things that I wanted to change. See the last four commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for these changes! It's a lot of progress for these modules.
camera
and mobject.graphing
@@ -26,6 +28,12 @@ | |||
def __init__(self, custom_labels: bool = False): | |||
self.custom_labels = custom_labels | |||
|
|||
@overload | |||
def function(self, value: float) -> float: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
def function(self, value: float) -> float: ... | ||
|
||
@overload | ||
def function(self, value: np.ndarray) -> np.ndarray: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
Overview: What does this pull request change?
Adding type annotations for files in mobject/graphing/.* and camera/.*