Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#11383 closed defect (fixed)

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

Reported by: slabbe Owned by: slabbe
Priority: major Milestone: sage-5.1
Component: graphics Keywords: beginner, sd31
Cc: kcrisman Merged in: sage-5.1.beta2
Authors: Ryan Grout, Itai Bar-Natan, Jan Pöschko Reviewers: Itai Bar-Natan, Karl-Dieter Crisman, André Apitzsch, Jan Pöschko
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by aapitzsch)

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

Attachments (4)

trac_11383_color_cmp.patch (1.8 KB) - added by ryan 10 years ago.
color testing v3
trac_11383_color_cmp_hash.patch (1.7 KB) - added by poeschko 9 years ago.
color comparison and hashing
improved-colors-patch.patch (2.1 KB) - added by itaibn 9 years ago.
trac 11383 eq ne hash
trac_11383.patch (3.0 KB) - added by aapitzsch 9 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 10 years ago by slabbe

  • Description modified (diff)
  • Owner changed from jason, was to slabbe

comment:2 Changed 10 years ago by kcrisman

  • Cc kcrisman added

comment:3 follow-up: Changed 10 years ago by 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)?

comment:4 Changed 10 years ago by ryan

  • Authors set to Ryan Grout
  • Keywords sd31 added
  • Status changed from new to needs_review

comment:5 in reply to: ↑ 3 Changed 10 years ago by 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

comment:6 Changed 10 years ago by kini

  • Status changed from needs_review to needs_work

<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).)

Changed 10 years ago by ryan

color testing v3

comment:7 Changed 10 years ago by 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".

comment:8 follow-up: Changed 10 years ago by 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

comment:9 in reply to: ↑ 8 Changed 10 years ago by slabbe

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

Oh! Interesting!

comment:10 Changed 10 years ago by kini

Ping - anyone doing anything with this?

comment:11 Changed 10 years ago by 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

    diff --git a/sage/plot/colors.py b/sage/plot/colors.py
    a b def rgbcolor(c, space='rgb'): 
    364364# For backward compatibility.
    365365to_mpl_color = rgbcolor
    366366
     367from sage.structure.unique_representation import UniqueRepresentation
    367368
    368 class Color(object):
     369class Color(UniqueRepresentation):
     370    @staticmethod
     371    def __classcall__(self, r='#0000ff', g=None, b=None, space='rgb'):
     372        if isinstance(r, list):
     373            r = tuple(r)
     374        return super(Color, self).__classcall__(self, r, g, b, space)
     375
    369376    def __init__(self, r='#0000ff', g=None, b=None, space='rgb'):
    370377        """
    371378        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.)

comment:12 Changed 9 years ago by 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

comment:13 Changed 9 years ago by 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.

Changed 9 years ago by poeschko

color comparison and hashing

comment:14 Changed 9 years ago by 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.

comment:15 Changed 9 years ago by 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.

Changed 9 years ago by itaibn

trac 11383 eq ne hash

Changed 9 years ago by aapitzsch

comment:16 Changed 9 years ago by aapitzsch

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Patch is needed for #12904.

comment:17 follow-up: Changed 9 years ago by 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.

Last edited 9 years ago by kcrisman (previous) (diff)

comment:18 Changed 9 years ago by 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.

comment:19 in reply to: ↑ 17 Changed 9 years ago by 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.

comment:20 follow-up: Changed 9 years ago by kcrisman

  • Authors changed from Ryan Grout to Ryan Grout, Itai Bar-Natan, Jan Pöschko
  • Reviewers set to Itai Bar-Natan, Karl-Dieter Crisman, André Apitzch, Jan Pöschko

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.

comment:21 in reply to: ↑ 20 Changed 9 years ago by 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.`

comment:22 Changed 9 years ago by kcrisman

  • Status changed from needs_review to positive_review

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

comment:23 Changed 9 years ago by 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.)

comment:24 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.1.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 Changed 9 years ago by jdemeyer

  • Reviewers changed from Itai Bar-Natan, Karl-Dieter Crisman, André Apitzch, Jan Pöschko to Itai Bar-Natan, Karl-Dieter Crisman, André Apitzsch, Jan Pöschko
Note: See TracTickets for help on using tickets.