-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Introduce seed in random_color
method to produce colors deterministically
#4265
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?
Conversation
for more information, see https://pre-commit.ci
Referencing issue #1671 I’m open to further discussion to fine-tune the implementation. For example, we could consider using a singleton instance in the default (non-seeded) case instead of creating a new object each time. If the overall approach looks good, I’m happy to iterate further and refine the PR for production readiness. |
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 this is a nice proposal, and a good resolution for this over-due issue. This approach would still work (or at least, could be made to work) in case we ever decide to introduce a module-global seed (ManimConfig.random_seed
...).
Could you please:
- double check that the line in the docstring, L1515 is not too long and break it if otherwise,
- add a PARAMETERS-section to the class docstring where the seed argument of the initializer is documented,
- and add the fixed-seed example from the description either as a unit test (together with the other color module tests) or as a doctest in an EXAMPLES section of the docstring? (I have a slight preference for the latter, as it simultaneously documents the usage of the generator class.)
Thank you for your contribution!
Great! |
May I add a little suggestion: I now often use Could you consider to add an additional, optional parameter to the One additional question: since |
@uwezi Update: |
@behackl please review and suggest anything else that needs to be changed. |
) -> None: | ||
self.choice = random.choice if seed is None else random.Random(seed).choice | ||
|
||
from manim.utils.color.manim_colors import _all_manim_colors |
Check notice
Code scanning / CodeQL
Cyclic import Note
manim.utils.color.manim_colors
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.
Looks good! I've left a hand full of suggestions to improve how the docstrings are rendered, please take a look.
I'll also push one additional commit in a second to fix the rendering of the code snippets.
Other than that, this seems good to go for me. Let me know once you've checked my suggested changes!
""" | ||
A generator for producing random colors from a given list of Manim colors, |
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.
""" | |
A generator for producing random colors from a given list of Manim colors, | |
"""A generator for producing random colors from a given list of Manim colors, |
optionally in a reproducible sequence using a seed value. | ||
When initialized with a specific seed, this class produces a deterministic | ||
sequence of `ManimColor` instances. If no seed is provided, the selection is |
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.
sequence of `ManimColor` instances. If no seed is provided, the selection is | |
sequence of :class:`.ManimColor` instances. If no seed is provided, the selection is |
Parameters | ||
---------- | ||
seed : int | None, optional |
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.
type annoations are taken from the actual type hints, no need to add them twice
seed : int | None, optional | |
seed |
A seed value to initialize the internal random number generator. | ||
If None (default), colors are chosen using the global random state. | ||
sample_colors : list[ManimColor], optional |
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.
sample_colors : list[ManimColor], optional | |
sample_colors |
---------- | ||
seed : int | None, optional | ||
A seed value to initialize the internal random number generator. | ||
If None (default), colors are chosen using the global random state. |
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.
If None (default), colors are chosen using the global random state. | |
If ``None`` (the default), colors are chosen using the global random state. |
If None (default), colors are chosen using the global random state. | ||
sample_colors : list[ManimColor], optional | ||
A custom list of Manim colors to sample from. Defaults to the full Manim color palette. |
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.
A custom list of Manim colors to sample from. Defaults to the full Manim color palette. | |
A custom list of Manim colors to sample from. Defaults to the full Manim | |
color palette. |
manim/utils/color/core.py
Outdated
Examples | ||
-------- | ||
Without a seed (non-deterministic): |
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.
The actual formatting for the example blocks need to be slightly adapted to make the documentation render correctly. I'll push a commit to take care of it. 👍
""" | ||
Returns the next color from the configured color list. |
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.
""" | |
Returns the next color from the configured color list. | |
"""Return the next color from the configured color list. |
One additional thing I just noticed: there is a warning in the docstring of the |
Overview: What does this pull request change?
RandomColorGenerator
, which enables deterministic generation of colors from Manim's color palette using an optional seed.random_color()
function to use this class internally, preserving existing behavior and output style.Motivation and Explanation: Why and how do your changes improve the library?
The existing
random_color()
function uses Python’s globalrandom
module, which makes the results non-deterministic and unsuitable for use cases like tests or reproducible animations.This PR introduces
RandomColorGenerator
, a utility that allows users to generate a repeatable sequence of random Manim colors by initializing it with a specific seed. The sequence is generated statefully—each call to.next()
returns the next color in that seeded sequence. Here’s an example:Meanwhile, the
random_color()
function continues to behave the same:This ensures full backward compatibility, with zero impact on existing code using
random_color()
.Further Information and Comments
Based on the discussion in the related issue, this design addresses two key requirements:
random_color()
API, which is widely used.Importantly, we do not add a
seed
parameter torandom_color()
to avoid confusion around stateless vs stateful behavior. Instead, theRandomColorGenerator
class provides an explicit, opt-in mechanism for seeded, stateful random color generation—making the intended behavior clear to users.This separation keeps the API clean while offering more control to advanced users who need it.
Reviewer Checklist