Add type annotations to three_dimensions.py
#4497
Conversation
chopan050
left a comment
chopan050
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
I left suggestions on the code. Most of them are minor changes, but there's a specific suggestion which is probably controversial and might require some discussion: making the checkerboard_colors None instead of False. Maybe it's better to simply use Literal[False] in this PR, discuss the topic, and then possibly change it to None in a follow-up PR?
| checkerboard_colors: Sequence[ParsableManimColor] | Literal[False] = [ | ||
| BLUE_D, | ||
| BLUE_E, | ||
| ], |
There was a problem hiding this comment.
More generally, checkerboard_colors could be any Iterable.
Based on how checkerboard_colors is used in the code, it seems more proper to use None instead of bool or Literal[False]. There are some caveats to that, however: there are some parts where the default value is explicitly False and making this change would require changing those default values as well, so feel free to disagree with me here.
| checkerboard_colors: Sequence[ParsableManimColor] | Literal[False] = [ | |
| BLUE_D, | |
| BLUE_E, | |
| ], | |
| checkerboard_colors: Iterable[ParsableManimColor] | None = [BLUE_D, BLUE_E], |
| u_range: Sequence[float] = [0, 1], | ||
| v_range: Sequence[float] = [0, 1], | ||
| resolution: Sequence[int] = 32, | ||
| resolution: Sequence[int] | int = 32, |
There was a problem hiding this comment.
Minor nitpick: I would prefer to have int first, since it's the simplest data type.
Since the sequence is always expected to contain 2 integers, tuple[int, int] seems to be more fitting.
Since
| resolution: Sequence[int] | int = 32, | |
| resolution: int | tuple[int, int] = 32, |
| if checkerboard_colors is False: | ||
| self.checkerboard_colors = checkerboard_colors | ||
| else: | ||
| self.checkerboard_colors = [ManimColor(i) for i in checkerboard_colors] |
There was a problem hiding this comment.
Following the previous suggestion, this could be something like:
| self.checkerboard_colors = [ManimColor(i) for i in checkerboard_colors] | |
| self.checkerboard_colors: list[ManimColor] | None | |
| if checkerboard_colors is None: # if not checkerboard_colors: | |
| self.checkerboard_colors = checkerboard_colors | |
| else: | |
| self.checkerboard_colors = [ManimColor(i) for i in checkerboard_colors] |
or:
| self.checkerboard_colors = [ManimColor(i) for i in checkerboard_colors] | |
| self.checkerboard_colors = ( | |
| [ManimColor(i) for i in checkerboard_colors] | |
| if checkerboard_colors is not None else None # if checkerboard_colors else None | |
| ) |
Note, however, that this might be a breaking change if people still passes checkerboard_colors=False, so you could consider the commented statements instead.
| u_range: Sequence[float] = [0, 1], | ||
| v_range: Sequence[float] = [0, 1], |
There was a problem hiding this comment.
Based on the code which always expects them to have two floats, would it be possible for these two to be tuple[float, float]?
| u_range: Sequence[float] = [0, 1], | |
| v_range: Sequence[float] = [0, 1], | |
| u_range: tuple[float, float] = (0, 1), | |
| v_range: tuple[float, float] = (0, 1), |
| u_res = v_res = self.resolution | ||
| else: | ||
| u_res, v_res = res | ||
| u_res, v_res = self.resolution[0:2] |
There was a problem hiding this comment.
By having resolution typed as tuple[int, int] by a previous suggestion, this slicing would no longer be necessary:
| u_res, v_res = self.resolution[0:2] | |
| u_res, v_res = self.resolution |
| self._current_theta = theta | ||
| self._current_phi = phi | ||
|
|
||
| def set_direction(self, direction: np.ndarray) -> None: |
There was a problem hiding this comment.
We could make this a Vector3DLike if we later convert it to a NumPy array inside the method:
| def set_direction(self, direction: Vector3DLike) -> None: |
| checkerboard_colors: Sequence[ParsableManimColor] | Literal[False] = False, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| self.direction = direction |
There was a problem hiding this comment.
This makes sure that the direction is copied and not referenced. References have caused some bugs in Manim:
| self.direction = direction | |
| self.direction = np.array(direction) |
| direction | ||
| The direction of the apex. | ||
| """ | ||
| self.direction = direction |
There was a problem hiding this comment.
Same as with the constructor: make sure to copy the direction rather than referencing it.
| self.direction = np.array(direction) |
| self.direction = direction | ||
| self._rotate_to_direction() | ||
|
|
||
| def get_direction(self) -> np.ndarray: |
There was a problem hiding this comment.
| def get_direction(self) -> Vector3D: |
| return self.direction | ||
|
|
||
| def _set_start_and_end_attributes(self, direction): | ||
| def _set_start_and_end_attributes(self, direction: Vector3DLike) -> None: |
There was a problem hiding this comment.
This is an internal method where direction is going to be exclusively a NumPy array. Having it be something else like a list will raise errors on the first line, where it's multiplied by a float.
| def _set_start_and_end_attributes(self, direction: Vector3DLike) -> None: | |
| def _set_start_and_end_attributes(self, direction: Vector3D) -> None: |
chopan050
left a comment
chopan050
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
I left suggestions on the code. Most of them are minor changes, but there's a specific suggestion which is probably controversial and might require some discussion: making the checkerboard_colors None instead of False. Maybe it's better to simply use Literal[False] in this PR, discuss the topic, and then possibly change it to None in a follow-up PR?
2 participants
Overview: What does this pull request change?
Motivation and Explanation: Why and how do your changes improve the library?
Links to added or changed documentation pages
Further Information and Comments
Reviewer Checklist