vaab / colour

Python color representations manipulation library (RGB, HSL, web, ...)
BSD 2-Clause "Simplified" License
322 stars 41 forks source link

Build an arbitrary color from an input string #6

Closed multani closed 11 years ago

multani commented 11 years ago

This allows to quickly build the same color from anything which can be turned into a string.

Use case: you have a dataset for a graph, where the name of each set is a defined string. You don't really care about the color generated, but you still want something consistent from the same set, without fumbling too much about making some kind of color. Here you are, just use colour.from_string(title) and you are done.

vaab commented 11 years ago

Thanks for the contribution. I'm not merging directly your branch mainly because it bumped into other concerns that should be tackled also (see issue https://github.com/vaab/colour/issues/3 ).

Besides, I may have to tweak some API choices you've introduced (mainly the name 'from_string' which bumps in my mind with etree.from_string). I was thinking about::

>>> foo_color = Color(pick_for=foo)

foo being any object that is hashable.

Additionally, there are lot of different way to pick color. Some of them might even be better for your case. Have you considered that randomizing RGB components will give more often colors with medium luminance ? If you want to have visually different colors, you might consider randomizing only "hue", and constrain "luminance" to 2 or 3 steps (0.3, 0.6, and 1.). Also, using random color is maybe not a good options, as visual closeness is often easily obtained, and we could think of a color picker that would compute the furthest color with all the previous ones.

As I can understand that your suggestion is perfectly sufficient for your case, I would like to give the developper using Color picker a way to customize the way color are generated for a given object. With a sane default of course, which could be your proposed color picker.

Was thinking of::

>>> foo_color = Color(pick_for=foo, picker=my_color_picker)

This last API suggestion doesn't convince me, because when implementing a picker, we would have to spawn a color object, so I can't see a difference between a color picker and a color factory (as the class Color is).

As a last comment on your implementation, I'm not sure if the requirement of the object being "stringable" is usefull: it seems to me that it is redundant with the hash mechanism in python objects. Maybe I didn't understand your case, so please develop this part so I might catch what are your concerns.

However, this IS a great idea and I like the algorithm you've chosen for choosing colors in the RGB space.

Many thanks !

multani commented 11 years ago

On Thu, Jun 20, 2013 at 11:42:29PM -0700, Valentin Lab wrote:

Thanks for the contribution. I'm not merging directly your branch mainly because it bumped into other concerns that should be tackled also (see issue https://github.com/vaab/colour/issues/3 ).

Well, I just implemented __eq__ for convenience while testing, if it's a problem, I can probably remove it from my patch.

Besides, I may have to tweak some API choices you've introduced (mainly the name 'from_string' which bumps in my mind with etree.from_string). I was thinking about::

foo_color = Color(pick_for=foo)

Hmm, although I've no problem changing the name of from_string, I'm not very keen to add another kind of Color constructor which is already quite complex (ie. accepts many kind of arguments), and it would also have to deal with argument conflicts (what would be a Color(Color("red"), rgb=(0.5, 0.5, 0.5), hsl=(0.3, 0.2, 0.1), pick_for="foo bar") ?)

foo being any object that is hashable.

Cf. below.

Additionally, there are lot of different way to pick color. Some of them might even be better for your case. Have you considered that randomizing RGB components will give more often colors with medium luminance ? If you want to have visually different colors, you might consider randomizing only "hue", and constrain "luminance" to 2 or 3 steps (0.3, 0.6, and 1.). Also, using random color is maybe not a good options, as visual closeness is often easily obtained, and we could think of a color picker that would compute the furthest color with the previous one.

I'm not a color expert, so if you have a better implementation to pick those colors, I'm open for it.

As I can understand that your suggestion is perfectly sufficient for your case, I would like to give the developper using Color picker a way to customize the way color are generated for a given object. With a sane default of course, which could be your proposed color picker.

Was thinking of::

>>> foo_color = Color(pick_for=foo, picker=my_color_picker)

This last API suggestion doesn't convince me, because when implementing a picker, we would have to spawn a color object, so I can't see a difference between a color picker and a color factory (as the class Color is).

Well, I think it goes out of the scope of what I'm proposing. (Basically, my use case what just to give some colors to data I was plotting, and I was just thinking "eh, I have 'something', give me color. Hey, I have the same 'thing', can you give me a color? Oh cool it's the same. Hey, I have 'something else', can you give me a color?", etc.)

I can see some use cases for this color picker object, but that I think this is subject for another patch (and I believe you will have to reverse your object dependencies, ie. having my_color_picker = ColorPicker(); my_color_picker.pick_for(foo) etc.)

As a last comment on your implementation, I'm not sure if the requirement of the object being "stringable" is usefull: it seems to me that it is redundant with the hash mechanism in python objects. Maybe I didn't understand your case, so please develop this part so I might catch what are your concerns.

I just needed a way to build up a color from "something". Being hashable or stringable is probably just an implementation detail. If you don't like forcing to have a string as input, the conversion can still be done inside this constructor. As for using the hash mechanism, my only objection would probably be that everything but a few objects are hashable (you can't hash a dict for example), which might be a limitation the string-version doesn't have.

vaab commented 11 years ago

I did write some code to make my idea clearer for me. Would this implementation hurt ponies ?

https://github.com/vaab/colour/tree/wip

I wrote this before I read your comments. I don't feel like having broken your expectation, but did add the 2 lines needed in the small init of Color class. However, the color picker you provided is in the same scope but was just renamed. Apart from that, you can use directly the color picker, and I can use the previously discussed API...

I removed the concept of string, but you shouldn't notice it. If there is some differences, you could easily make your color picker with:

RGB_color_picker = lambda obj: Colour.RGB_color_picker(str(obj)) 

But I can't see any cases where it should be necessary.

Please note also that in that implementation, a color picker is a simple function that take any object as argument and must return something that Color could instantiate. This allows returning "red", "#ff0" ... or a Color instance for example.

multani commented 11 years ago

Looks good to me.

I don't have much objections about it, but my concern about hashing a dict is still valid though.

vaab commented 11 years ago

I'm not sure to properly understand your concern with dict hashing.

I understand that if you need to colorize different dicts, you'll have to provide a hash function of your own for your dict-like object.

Besides "str" representation of dict isn't to be relied upon, isn't it ? Key orders and Set orders make them subject to unreliable representation of them. And often, the __str__ is not implemented with the "identifier" responsibility in head. This is why this solution seems to me very specific to your current environment. Feel free to correct me.

Besides, if we have to compare reponsibility between __hash__ and __str__... it seems that __hash__ is the one in charge to give an "identifier" which is meant to be used as such, and __str__ doesn't seem to be as related to the charge of identifying the object.

I look into this next week to take time to consider all this. Thank for your contribution and your feedback. ;)

multani commented 11 years ago

I understand that if you need to colorize different dicts, you'll have to provide a hash function of your own for your dict-like object.

That would defeat the goal of having a simple way to get a color from anything, which was my intent in the very first place, although I understand there are other concerns with the string-way as you mentionned above. It also means that you will have to specify in your interface that objects from which you want to create a color need to be hashable.

Anyway, as far as I'm concerned, I'm only dealing with strings, and I guess I can still give a str() representation of my objects before feeding them to Color, even if it uses internally the hash mechanism.

So you're good to go as it.

vaab commented 11 years ago

Here are more information I want to store in this thread for future references.

In python, __hash__ is meant to create a hashing key and is not provided for list, set, dict because they are mutable, and that giving a key to a mutable object is not a good idea:

http://stackoverflow.com/questions/7842049/are-mutable-hashmap-keys-a-dangerous-practice

I can understand @multani 's concerns about using __hash__.

Associating a color to an object is a way to identify it. Additional property of @multani 's implementation is that color choosed for an object is the same between runs (provided they have the same string representation). We could imagine other color picker which would not pick same color at each run. My last implementation uses "hash" and defaults to __str__ if hash fails, and I still don't feel it as a good solution. This is why I added a pick_key attribute in the Color kwargs instantiation (yes, another one. I'm also concerned with the kwargs bloating of this Color instantiation, but feel like there is some sense to continue to give that API to some developper.)

This last refactor removes the burden of finding a key from the picker itself. So we are using the first implementation of @multani here for the picker.

Only Color API changed and added this pick_key attribute.

You could then:

>>> Color(pick_for="", pick_key=str)

Or also:

>>> Color(pick_for=3, pick_key=id)
>>> Color(pick_for=foo, pick_key=hash)

Don't forget that with the last commit, I added a factory system which helps configure defaults attribute value for an application, this could be of some use if you already now what pick_key you need.