Skip to content

Rewrite color_gradient to always return a list of ManimColors #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

Merged
merged 4 commits into from
Aug 11, 2025

Conversation

henrikmidtiby
Copy link
Contributor

Overview: What does this pull request change?

This PR introduces a new method color_gradient_as_list with similar functionality as color_gradient but that is guaranteed to always return a list of ManimColor objects. Then all calls to color_gradient in the code base was replaced with calls to color_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 a ManimColor.

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

  • 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

@github-project-automation github-project-automation bot moved this to 🆕 New in Dev Board Aug 7, 2025
@henrikmidtiby henrikmidtiby marked this pull request as ready for review August 7, 2025 04:30
Copy link
Contributor

@chopan050 chopan050 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 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, then color_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 of length_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, a map, 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 an IndexError.
  • Broaden the type of reference_colors from Sequence to Iterable by avoiding the indexing and length calculation of reference_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:

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Aug 9, 2025
@henrikmidtiby henrikmidtiby mentioned this pull request Aug 10, 2025
3 tasks
@henrikmidtiby
Copy link
Contributor Author

@chopan050 Thanks for your suggestions!
To me it is much better to make the color_gradient be more consistent instead of adding a new copy of this function.

@chopan050 chopan050 changed the title Made a copy of color_gradient that is guaranteed to return a list. Rewrite color_gradient to always return a list of ManimColors Aug 11, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks!

@github-project-automation github-project-automation bot moved this from 👀 In review to 👍 To be merged in Dev Board Aug 11, 2025
@chopan050 chopan050 merged commit 5a2b338 into ManimCommunity:main Aug 11, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this from 👍 To be merged to ✅ Done in Dev Board Aug 11, 2025
@henrikmidtiby henrikmidtiby deleted the RestructureColorGradient branch August 11, 2025 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants