Color('red') == Color('red') returns False

[slabbe]

I would like this to be True:

sage: Color('red') == Color('red')
False

and these as well:

sage: Color((1,0,0)) == Color('red')  
False                                 
sage: Color((1,0,0)) == Color((1,0,0))
False                                 

Apply trac_11383.patch​

[ryan]

how picky do we want a comparison to be? Should it be more of a perceptive comparison (similar colors evaluate to True) or more of a numerically exact comparison (the colors must be _exactly_ the same to evaluate to True)?

[slabbe]

Replying to ryan:

how picky do we want a comparison to be? Should it be more of a perceptive comparison (similar colors evaluate to True) or more of a numerically exact comparison (the colors must be _exactly_ the same to evaluate to True)?

Good question. I was thinking the second (more picky).

Will look at the patch tomorrow.

Sébastien

[kini]

<rgrout> when comparing colors, do we compare the floating point tuple or should we scale by 255 and round and then compare?

<rgrout> so I guess I'm asking how picky do we want a comparison to be? Should it be more of a perceptive comparison or more of a numerically exact comparison?

<kini> perceptual similarity is not transitive

<kini> though rounding to the closest 1/256 is not exactly a perceptual comparison

I agree with slabbe. The patch gives me doctest errors. Please fix them! (Specifically 'green' is not (0,1,0).)

[ryan]

color testing v3

[slabbe]

Another reason to be picky is the already present hashing behavior. Very close colors get different hashing value, so we want those color not to be equal. Otherwise we must change the hash method which is not a good idea I think.

sage: hash(Color((1,0,0)))          
188337232                           
sage: hash(Color((1.0,0,0)))        
188338128                           
sage: hash(Color((1.0,0.0,0)))      
188337616                           
sage: hash(Color((1.0,0.0,0.0)))    
188337616                           
sage: hash(Color('red'))            
188337360                           
sage: hash(Color((0.99999,0.0,0.0)))
188614128                                                           

From the ​Python Language Reference : "The only required property is that objects which compare equal have the same hash value".

[kini]

Color hashing is already broken in terms of any possible sane equality comparison:

sage: c = Color('red')
sage: hash(c)
87590352
sage: hash(Color('red'))
87590672
sage: hash(Color('red'))
87590224
sage: 

Something else to note: Sage is not SAGE, it is Sage! Please help to eradicate SAGE from the codebase rather than introducing new instances of it! :P

[slabbe]

Color hashing is already broken in terms of any possible sane equality comparison:

Oh! Interesting!

[kini]

Ping - anyone doing anything with this?

[jhpalmieri]

Why not change the hash method? It could be something like

    def __hash__(self):
        return hash(self.__rgb)

(This would need doctests, obviously.)

Alternatively, what about a patch like this:

sage/plot/colors.py

@@ -364 +364 @@
 # For backward compatibility.
 to_mpl_color = rgbcolor
 
+from sage.structure.unique_representation import UniqueRepresentation
 
-class Color(object):
+class Color(UniqueRepresentation):
+    @staticmethod
+    def __classcall__(self, r='#0000ff', g=None, b=None, space='rgb'):
+        if isinstance(r, list):
+            r = tuple(r)
+        return super(Color, self).__classcall__(self, r, g, b, space)
+
     def __init__(self, r='#0000ff', g=None, b=None, space='rgb'):
         """
         An Red-Green-Blue (RGB) color model color object.  For most

(The __classcall__ method might need a bit more cleaning up, and it needs documentation and doctests.)

[ryan]

I think that redefining hash would work. Is there an example where the already published patch fails. I don't understand how comparing the hashes is better than directly comparing xx._Color__rgb==tt._Color__rgb

sage: a,b
(0.8216543768062763, 0.3482611655758555)
sage: print a,b
0.821654376806 0.348261165576
sage: b = a-.000000000000000000001
sage: b
0.821654376806276
sage: a
0.8216543768062763
sage: a==b
True
sage: xx = Color((a,b,0))
sage: tt = Color((b,a,0))
sage: xx==tt
True
sage: hash(xx)==hash(tt)
False
sage: hash(xx._Color__rgb)==hash(tt._Color__rgb)
True

[poeschko]

As suggested, I added a new __hash__ method (including doctests) that uses __rgb just like __eq__ and did some minor code improvements in the original patch. Note that this is a rather "picky" approach, e.g.

sage: Color((0.99999,0.0,0.0)) == Color((1,0,0))
False

All tests pass (in Sage 5.0.beta11 under Mac OS X 10.5.8/Intel), so I think this works fine.

By the way (@kini), the issue with green is that

sage: Color('green').rgb()
(0.0, 0.5019607843137255, 0.0)

so this is bound not be equal to Color((0,1,0)). Color('lime') is RGB green.

[poeschko]

color comparison and hashing

[itaibn]

I noticed that these patches don't define a __ne__ operator, and so the != operator doesn't work.

sage: Color('blue') != Color('blue')
True

I'm working on a patch to fix this.

[itaibn]

Now I notice that the __eq__ function returns an error when comparing a Color with something that isn't a Color. I don't think that is the intended behaviour.

[itaibn]

trac 11383 eq ne hash

[aapitzsch]

Patch is needed for #12904.

[kcrisman]

I'm a little confused about a few things.

• How many authors do we have, how many reviewers? I'm not going to try to diff out each of the patches here; can each contributor put their name where it belongs (maybe both spots sometimes)? For instance, it seems like aapitzsch acted more in a reviewer role on itaibn's patch.
• sage: Color(0.2,0.3,0.2) == False seems like an odd doctest, but maybe it's just to check 'garbage' input? sage: Color(0.2,0.3,0.2) == 0.2 or sage: Color(0.2,0.3,0.2) == x^2+1 test the same thing, but make it more obvious that the test isn't a typo and is covering a branch of the code, like the 23 comparison in ne.
• This is just my preference, but in view of comment:7 it would be nice to have

  sage: Color((1,0,0)) == Color((1.0,0,0))
  True
  

  although of course the hash issue is taken care of and tested.

Anyway, none of this is really important enough to hold it up unless one of the many contributors to the ticket has an idea about them!

Any other things that could be come up here? This is needed for the eminently more practical #12904 so it would be nice to get positive review.

[kcrisman]

The only other thing I can think of is what behavior we want for

sage: Color('red') > Color('yellow')

I figure they should all be False? or an error? But actually we now get

sage: Color('red') > Color('red')
True
sage: Color('red') > Color('yellow')
True

Hmm... I feel like this is needs work, though this is the previous behavior. But if we're going to fix this, it should really be fixed. What do you all think?

Ironically, whoever first added the very nice docstring formatting may be disappointed - although this is in the reference manual, double underscore methods don't show up! But I appreciate it.

[itaibn]

Replying to kcrisman:

• How many authors do we have, how many reviewers? I'm not going to try to diff out each of the patches here; can each contributor put their name where it belongs (maybe both spots sometimes)? For instance, it seems like aapitzsch acted more in a reviewer role on itaibn's patch.

My role was to add the __ne__ function, improve the __eq__ function so that it doesn't return an error when comparing a Color and a non-Color, and doctest such comparisons (including the Color(0.2,0.3,0.2) == False test). By looking at the different patches it appears as though ryan wrote the __eq__ function (though the 'v3' in the description suggests there's more to it), poeschko wrote the __hash__ function, and aapitzsch improved the line wrapping in my patch and improved some unrelated parts of the file.

[kcrisman]

Okay. Any opinions on the Color('red') > Color('red') issue? I feel like this is pretty much the same issue as the original ticket, though perhaps not as likely to be useful in "production" code.

[kcrisman]

Okay. Any opinions on the Color('red') > Color('red') issue? I feel like this is pretty much the same issue as the original ticket, though perhaps not as likely to be useful in "production" code.

I've made a followup to this at #12999. Assuming everything still applies, I think it's better to have this in than quibble.`

[kcrisman]

Tests on this pass. If kini really wants to run full doctests he is welcome to.

[kini]

patchbot: apply trac_11383.patch

(The above should cause a couple patchbots to test this correctly, and fully, on 5.0 and 5.1.beta0.)