Skip to content

random_color() and random_bright_color() should have a parameter random_seed #1671

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

Open
kolibril13 opened this issue Jun 11, 2021 · 23 comments · May be fixed by #4265
Open

random_color() and random_bright_color() should have a parameter random_seed #1671

kolibril13 opened this issue Jun 11, 2021 · 23 comments · May be fixed by #4265
Labels
enhancement Additions and improvements in general good first issue Good for newcomers

Comments

@kolibril13
Copy link
Member

kolibril13 commented Jun 11, 2021

As it is always good to reproduce randomness, these two functions should have a parameter seed.
https://docs.manim.community/en/stable/_modules/manim/utils/color.html#random_color
Then it should be possible to just init that seed.
https://towardsdatascience.com/stop-using-numpy-random-seed-581a9972805f
EDIT: Second link does not apply here.

@kolibril13 kolibril13 added enhancement Additions and improvements in general good first issue Good for newcomers labels Jun 11, 2021
@behackl
Copy link
Member

behackl commented Jun 12, 2021

The link you have posted regarding the numpy random seed doesn't really apply for the random_color function, it doesn't use numpy's random module.

@behackl behackl linked a pull request Jun 12, 2021 that will close this issue
12 tasks
@kolibril13
Copy link
Member Author

True, sorry!
I got confused, because there is also a choise function in numpy.
Here is the correct one:
https://docs.python.org/3/library/random.html#bookkeeping-functions

@RaghavPrabhakar66
Copy link
Contributor

Hi, Can I take this issue?

@kolibril13
Copy link
Member Author

Hi, Can I take this issue?

there was already a pull request one month ago here: #1676
but it seems to be stuck.
@RaghavPrabhakar66 : Feel free to review this pr, or, in case that you have another idea, you can also make a new pr

@RaghavPrabhakar66
Copy link
Contributor

@kolibril13 I reviewed PR #1676, it works but I don't think we need if-else code statement, we can do it directly also.
Like rng = random.Random(random_seed).
Some Experiments:

>>> from manim.utils.color import random_color, random_bright_color
>>> random_color()
'#FFEA94'
>>> random_color()
'#00FF00'
>>> random_color(42)
'#ECABC1'
>>> random_color(random_seed=42)
'#ECABC1'
>>> random_color()
'#FFF1B6'
>>> random_bright_color()
<Color #ddd>
>>> random_bright_color()
<Color #fbd0d1>
>>> random_bright_color(42)
<Color #f5d5e0>
>>> random_bright_color(42)
<Color #f5d5e0>

So should I create a new pr?

@behackl
Copy link
Member

behackl commented Jul 20, 2021

So should I create a new pr?

Hey, I have reviewed that PR already a while ago, but the fundamental issue noted in #1676 (comment) has not been discussed or resolved yet. Removing the if-else will also not change that.

@RaghavPrabhakar66
Copy link
Contributor

I don't think that this behavior is expected, but I also don't really have a good / simple solution for this. I think that essentially, we'd have to check whether a RNG with the given seed has already been initialized, and then use the existing one – or create one.

I don't know if I understood it correctly. What is the expected behavior? Is it if random.Random() is already defined with random_seed then no need to create RNG Object and If not then, we create RNG Object and set it's seed to random_seed.

I looked into it and I don't think its possible with python random module. Random does give us function such getstate() and setstate() which can be used to get info on its current state. getstate() function returns a list of size 624 and I dont think we can extract seed from that.

>>> import random
>>> random.getstate()[1][0]
2147483648
>>> random.seed(42)
>>> random.getstate()[1][0]
2147483648

Though this can be done in Numpy random module as np.random.get_state()[1][0] returns random_seed.

>>> import numpy as np
>>> np.random.get_state()[1][0]
2147483648
>>> np.random.seed(42)
>>> np.random.get_state()[1][0]
42
>>> np.random.randint(0, 100)
51
>>> np.random.randint(0, 100)
92
>>> np.random.randint(0, 100)
14
>>> np.random.get_state()[1][0]
723970371

But it also changes after some tries, so we can't determine the seed. I found something on stackoverflow related to it:

In summary on np.random.seed(s):
- get_state()[1][0] immediately after setting: retrieves s that exactly recreates the state
- get_state()[1][0] after generating numbers: may or may not retrieve s, but it will not recreate the current state (at get_state())
- get_state()[1][0] after generating many numbers: will not retrieve s. This is because pos exhausted its representation.
- get_state() at any point: will exactly recreate that point.
Lastly, behavior may also differ due to get_state()[3:] (and of course [0]).

So, I don't think we can retrieve current Random seed as the seed is used to update the internal state of the random number generator is not directly stored anywhere.

References:

@behackl
Copy link
Member

behackl commented Jul 20, 2021

Before we get into implementation details, we have to agree on what kind of behavior we should actually expect. Personally, I think that a situation like

>>> random_bright_color(42)
<Color #f5d5e0>
>>> random_bright_color(42)
<Color #f5d5e0>

is not desireable, because setting the seed then is equivalent to hard-coding one specific color. I would expect that repeatedly calling random_color(random_seed=42) continues to give random colors, but in a reproducible way.

You are right that the current seed of an initialized RNG cannot be retrieved -- but as it is mentioned in the SO answers you've posted, we can still store the seed of the RNG that has been initialized last somewhere. (Maybe in Manim's configuration, maybe "just" in the module; not sure about that.) Then, when random_color is called again, the passed seed can be compared to the stored one, and either the already initialized RNG can be used, or a new one can be initialized.

@RaghavPrabhakar66
Copy link
Contributor

Yeah, we can store seed in config file and compare it if users provide random_seed.

I would expect that repeatedly calling random_color(random_seed=42) continues to give random colors, but in a reproducible way.

I was thinking maybe we could use getstate() and setstate() functions. Like we could return the state of seed at that with getstate() and later on set it back to its previous state using setstate().

In the meanwhile, I can try working on storing it in config system.

@hickmott99
Copy link
Contributor

Has anyone closed this issue? Can I tackle it? @RaghavPrabhakar66 @behackl

@Phosphorescentt
Copy link
Contributor

I've had a look through all of the stuff discussed in this issue so far and I'm not sure the solution of globally storing a seed for the random function is ideal because it opens the door for other areas of the code to reference that same seed and then produce inconsistent results if the code is edited. Consider the two following snippets of code:

colour1 = random_color(42)
colour2 = random_color(42)
shape1 = random_shape(42)
shape2 = random_shape(42)
colour1 = random_color(42)
shape1 = random_shape(42)
colour2 = random_color(42)
shape2 = random_shape(42)

These two snippets, although only a trivial reordering of one another, would produce very different results (assuming that random_shape accesses the same seed/RNG) and I don't think it's immediately obvious that either random_color or random_shape should have side effects.

I can think of a few solutions right now, each of course with their own benefits and drawbacks:

  1. Store the seed for random_colour separately so that there's no risk of future code using it and causing issues.
  2. Make random_colour return a seed that can be stored by the end user and fed back into random_colour later on. In this solution, we would use a new instance of a RNG every time random_colour is called.
  3. Introduce a random_colours(n: int, seed: int = None) -> List[Color] function that returns a list of n random colours.

Comments on the above solutions:

  1. Would probably introduce some tech debt and encourage the storage for RNG seeds to become very far out of scope if other areas of the code wanted to do the same thing.
  2. Gives the end user the most control, but is by far the least ergonomic. Having to store 2 values from random_color every time it's run and then pass one of those parameters back into random_color is not ideal.
  3. This solution removes the non-deterministic behavour when reordering code and I think it's probably the best solution right now save for a full rewrite of the way that RNG is handled by Manim as a whole which is a conversation for a separate issue.

Let me know what you think of the above and we can discuss further before I actuallly write any code. Feel free to @ me (Phosphorescent#2786) in the Discord if you'd rather talk there :)

@KaylaMLe
Copy link

KaylaMLe commented Sep 4, 2023

Hi! I noticed that this issue has been open for a while. I'm looking to make my first contribution, so I don't have a lot of experience with the project, but I agree that creating a separate function seems like the best idea. Introducing non-deterministic behavior like that seems like a nightmare. Is anyone else still actively working on this issue, or can I go ahead and start implementing that approach?

@uwezi
Copy link
Contributor

uwezi commented Sep 4, 2023

just an additional comment: I'd suggest a parameter to exclude either certain colors or at least to optionally automatically exclude the current background color.

@KaylaMLe
Copy link

KaylaMLe commented Sep 4, 2023

That's a good point. I imagine re-rendering something until you get an actually visible color gets more difficult as you work with more colors. Actually, wouldn't the function have to exclude a range of very similar colors to avoid that situation?

@uwezi
Copy link
Contributor

uwezi commented Sep 4, 2023

That's a good point. I imagine re-rendering something until you get an actually visible color gets more difficult as you work with more colors. Actually, wouldn't the function have to exclude a range of very similar colors to avoid that situation?

I don't think so, since it anyway are not too many different colors from which the random colors are drawn - but It would probably not too difficult to find similar colors to exclude using hue and brightness.

@KaylaMLe
Copy link

KaylaMLe commented Sep 4, 2023

I think that goes beyond the scope of this issue, but it does sound like enough of a concern to deserve its own issue. I'm not too sure though. What do you think?

@uwezi
Copy link
Contributor

uwezi commented Sep 4, 2023

I think that goes beyond the scope of this issue, but it does sound like enough of a concern to deserve its own issue. I'm not too sure though. What do you think?

I am just thinking that it would be easiest to implement these things at the same time rather than doing surgery on this code several times in a row. And I also think that just excluding the current background is "good enough" - I reverted to random_bright_color() on a dark background, but the contrast between the different shades is very weak.

@KaylaMLe
Copy link

KaylaMLe commented Sep 4, 2023

Sounds good. I'm not experienced enough with Manim to really know how dificult/easy using these functions are

@MrDiver
Copy link
Collaborator

MrDiver commented Sep 4, 2023

I think that goes beyond the scope of this issue, but it does sound like enough of a concern to deserve its own issue. I'm not too sure though. What do you think?

I am just thinking that it would be easiest to implement these things at the same time rather than doing surgery on this code several times in a row. And I also think that just excluding the current background is "good enough" - I reverted to random_bright_color() on a dark background, but the contrast between the different shades is very weak.

Would you like to clarify what you mean by excluding "background" ? As per the color PR it uses the manim core colors for random colors. Or like the basic manim colors

@MrDiver
Copy link
Collaborator

MrDiver commented Sep 5, 2023

Nvm.

So the current goal of this issue becomes changing the behavior with the seed but I also think that the exclusion of a specific color range shouldn't be done in this. Because in the end it gets more complicated if you want to have a magic function which basically computes legible colors if you specify a background i actually think that one should be separate if we want to do it right we need to do some research what that actually means.
Because just excluding the exact background color won't really help in that situation even if i see where this comes from. But i think we should just stick to the seed for this ticket, so then yes we have to work on it twice but at least we do it right!

@MrDiver
Copy link
Collaborator

MrDiver commented Sep 5, 2023

@uwezi
Copy link
Contributor

uwezi commented Sep 9, 2023

I have a suggestion here - I created a small RandomColor object which contains its own instance of a random number generator which can be seeded and functions to pick a random color from a given set of colors, a color with a different hue from a given reference color or saturation/value or saturation/lightness pair.

class RandomColor(object):
    def __init__(self, seed = None) -> None:
        self.generator = np.random.RandomState(seed=seed)
        self.palette = ["#FFFFFF"]
    def setPalette(self, palette : Sequence[ParsableManimColor]):
        self.palette = palette
    def randomPaletteColor(self):
        return self.generator.choice(self.palette)    
    def randomHue(self, reference_color:ParsableManimColor = "#FF0000", saturation=None, lightness=None, value=None):
        if saturation != None:
            if lightness != None:
                rgb = colorsys.hls_to_rgb(h=self.generator.uniform(0,1),s=saturation,l=lightness)
            elif value != None:
                rgb = colorsys.hsv_to_rgb(h=self.generator.uniform(0,1),s=saturation,v=value)
            else:
                rgb = colorsys.hsv_to_rgb(h=self.generator.uniform(0,1),s=saturation,v=1)
        else:
            hsv = colorsys.rgb_to_hsv(*color_to_rgb(reference_color))
            rgb = colorsys.hsv_to_rgb(h=self.generator.uniform(0,1),s=hsv[1],v=hsv[2])
        return ManimColor(rgb)

usage:

class randTest(Scene):
    def construct(self):
        rndcol = RandomColor()
        rndcol.setPalette([RED,GREEN,BLUE,YELLOW])
        for i in range(len(rndcol.palette)):
            self.add(Rectangle(width=1, height=0.3, fill_opacity=1, color=rndcol.palette[i]).move_to([-6+i,3.5,0]))
        self.add(Square(side_length=0.6, fill_opacity=1, color=PURE_BLUE).move_to(6.6*LEFT+0*UP))
        self.add(Square(side_length=0.6, fill_opacity=1, color=RED).move_to(6.6*LEFT+1*DOWN))
        self.add(Square(side_length=0.6, fill_opacity=1, color="#4c855b").move_to(6.6*LEFT+2*DOWN))
        self.add(Square(side_length=0.6, fill_opacity=1, color="#e0d9c3").move_to(6.6*LEFT+3*DOWN))

        for x in np.linspace(-6,6.5,20):
            self.add(Dot(x*RIGHT+3*UP, radius=.3, color=rndcol.randomPaletteColor()))

            self.add(Dot(x*RIGHT+0*UP, radius=.3, color=rndcol.randomHue(PURE_BLUE)))
            self.add(Dot(x*RIGHT+1*DOWN, radius=.3, color=rndcol.randomHue(RED)))
            self.add(Dot(x*RIGHT+2*DOWN, radius=.3, color=rndcol.randomHue("#4c855b")))
            self.add(Dot(x*RIGHT+3*DOWN, radius=.3, color=rndcol.randomHue("#e0d9c3")))

image

@uwezi
Copy link
Contributor

uwezi commented Sep 9, 2023

Ok I continued to work on it, ready to suggest a pull-request, after some additional polishing. What does everyone/anyone think?

  • rndcol = RandomColor(seed=42) initializes an instance with its own random number seed
  • rndcol.randomColor() same as the current plain random_color()
  • rndcol.setPalette([RED,GREEN,BLUE,YELLOW]) sets an internal color palette for random choice
  • rndcol.randomPaletteColor(allow_repeat=False) picks a random color from the palette, avoiding immediate repetitions
  • rndcol.randomContrastColor(PURE_BLUE)) tries to pick contrasting colors with regard to hue, saturation and brightness
  • rndcol.randomHue(RED)) generates a random color with same saturation and brightness, but random hue
class randTest(Scene):
    def construct(self):
        rndcol = RandomColor(seed=42)
        rndcol.setPalette([RED,GREEN,BLUE,YELLOW])
        for i in range(len(rndcol.palette)):
            self.add(Rectangle(width=1, height=0.3, fill_opacity=1, color=rndcol.palette[i]).move_to([-4+i,3.5,0]))
        self.add(Tex(r"randomPaletteColor()", font_size=22).move_to([-7,3,0],aligned_edge=LEFT))
        self.add(Tex(r"randomColor()", font_size=22).move_to([-7,2.3,0],aligned_edge=LEFT))
        self.add(Rectangle(height=0.6, width=14, fill_opacity=1, color="#e0d9c3").move_to(7*LEFT+1.5*UP, aligned_edge=LEFT))
        self.add(Tex(r"randomContrastColor()", font_size=22,color=BLACK).move_to([-7,1.5,0],aligned_edge=LEFT))
        self.add(Rectangle(height=0.6, width=14, fill_opacity=1, color=PURE_BLUE).move_to(7*LEFT+0.5*UP, aligned_edge=LEFT))
        self.add(Tex(r"randomContrastColor()", font_size=22,color="#768040").move_to([-7,0.5,0],aligned_edge=LEFT))
        self.add(Square(side_length=0.6, fill_opacity=1, color="#FFFF00").move_to(5.5*LEFT+0.5*DOWN))
        self.add(Square(side_length=0.6, fill_opacity=1, color=RED).move_to(5.5*LEFT+1.5*DOWN))
        self.add(Square(side_length=0.6, fill_opacity=1, color="#4c855b").move_to(5.5*LEFT+2.5*DOWN))
        self.add(Square(side_length=0.6, fill_opacity=1, color="#e0d9c3").move_to(5.5*LEFT+3.5*DOWN))
        self.add(Tex(r"randomHue()", font_size=30,color="WHITE").rotate(90*DEGREES).move_to([-6.5,-2,0],aligned_edge=LEFT))

        for x in np.linspace(-4.5,6.5,15):
            self.add(Dot(x*RIGHT+3*UP, radius=.3, color=rndcol.randomPaletteColor()))

            self.add(Dot(x*RIGHT+2.3*UP, radius=.3, color=rndcol.randomColor()))
            self.add(Dot(x*RIGHT+1.5*UP, radius=.25, color=rndcol.randomContrastColor("#e0d9c3")))
            self.add(Dot(x*RIGHT+0.5*UP, radius=.25, color=rndcol.randomContrastColor(PURE_BLUE)))
            self.add(Dot(x*RIGHT+0.5*DOWN, radius=.3, color=rndcol.randomHue("#FFFF00")))
            self.add(Dot(x*RIGHT+1.5*DOWN, radius=.3, color=rndcol.randomHue(RED)))
            self.add(Dot(x*RIGHT+2.5*DOWN, radius=.3, color=rndcol.randomHue("#4c855b")))
            self.add(Dot(x*RIGHT+3.5*DOWN, radius=.3, color=rndcol.randomHue("#e0d9c3")))

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general good first issue Good for newcomers
Projects
Status: 🆕 New
Development

Successfully merging a pull request may close this issue.

8 participants