-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Made a copy of color_gradient
that is guaranteed to return a list.
#4380
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
base: main
Are you sure you want to change the base?
Made a copy of color_gradient
that is guaranteed to return a list.
#4380
Conversation
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.
Thanks for this!
I would like to have an opinion from @MrDiver, but, instead of making a new function, what I would do is modify the original color_gradient
to always return a list of length length_of_output
, because it's the most consistent option:
- IMO, if
length_of_output=0
, thencolor_gradient
should return an empty list[]
, not one color (as it originally did) nor a list containing a single color (as this PR is currently proposing). A list of length 0 is the most expected and intuitive result for something which asks for something oflength_of_output=0
. - According to all the uses of
color_gradient
around the source code, its returned value is always expected to be a list of colors, because the result is always iterated over immediately after, either via a for loop, amap
, or direct indexing.
There are also some small changes I would like to propose regarding color_gradient
:
- Handle better the case where there are 0
reference_colors
, because it currently crashes with anIndexError
. - Broaden the type of
reference_colors
fromSequence
toIterable
by avoiding the indexing and length calculation ofreference_colors
.
def color_gradient(
reference_colors: Iterable[ParsableManimColor],
length_of_output: int,
) -> list[ManimColor]:
if length_of_output == 0:
return []
parsed_colors = [ManimColor(color) for color in reference_colors]
# (delete this comment) Now we can get the length of parsed_colors instead of reference_colors!
num_colors = len(parsed_colors)
if num_colors == 0:
raise ValueError("Expected 1 or more reference colors. Got 0 colors.")
if num_colors == 1:
return parsed_colors * length_of_output
rgbs = [color.to_rgb() for color in parsed_colors]
# (delete this comment) Now comes the rest, but replace len(rgbs) with num_colors
alphas = np.linspace(0, num_colors - 1), length_of_output)
floors = alphas.astype("int")
alphas_mod1 = alphas % 1
# End edge case
alphas_mod1[-1] = 1
floors[-1] = num_colors - 2
return [
rgb_to_color((rgbs[i] * (1 - alpha)) + (rgbs[i + 1] * alpha))
for i, alpha in zip(floors, alphas_mod1)
]
Finally, there are some small requests I would like to do regarding some changes in the code:
@@ -92,7 +92,7 @@ def thin_func(num_points=num_points): | |||
|
|||
def set_color_by_gradient(self, *colors): | |||
self.rgbas = np.array( | |||
list(map(color_to_rgba, color_gradient(*colors, self.get_num_points()))), | |||
map(color_to_rgba, color_gradient_as_list(*colors, self.get_num_points())), |
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.
Doing something like
np.array(map(int, "34"))
returns array(<map object at [address]>, dtype=object)
instead of array([3, 4])
, which is what we want. We need to convert that map
to a list
before converting it to a NumPy array (another option would be using np.fromiter
, but let's keep consistency with the rest of the code):
map(color_to_rgba, color_gradient_as_list(*colors, self.get_num_points())), | |
list(map(color_to_rgba, color_gradient_as_list(*colors, self.get_num_points()))), |
@@ -124,7 +124,7 @@ def set_stroke_width(self, width: int, family: bool = True) -> Self: | |||
|
|||
def set_color_by_gradient(self, *colors: ParsableManimColor) -> Self: | |||
self.rgbas = np.array( | |||
list(map(color_to_rgba, color_gradient(*colors, len(self.points)))), | |||
map(color_to_rgba, color_gradient_as_list(*colors, len(self.points))), |
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.
map(color_to_rgba, color_gradient_as_list(*colors, len(self.points))), | |
list(map(color_to_rgba, color_gradient_as_list(*colors, len(self.points)))), |
Overview: What does this pull request change?
This PR introduces a new method
color_gradient_as_list
with similar functionality ascolor_gradient
but that is guaranteed to always return a list of ManimColor objects. Then all calls tocolor_gradient
in the code base was replaced with calls tocolor_gradient_as_list
.Motivation and Explanation: Why and how do your changes improve the library?
The intention with this PR is to make type annotations of the rest of the code base easier.
In some cases it is quite tricky to get around the special case where
color_gradient
returns aManimColor
.It should be considered if it is enough to alter the
color_gradient
method instead of introducing a new method with almost the same functionality.Links to added or changed documentation pages
Reviewer Checklist