#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: |
Description (last modified by )
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)
Change History (29)
comment:1 Changed 10 years ago by
- Description modified (diff)
- Owner changed from jason, was to slabbe
comment:2 Changed 10 years ago by
- Cc kcrisman added
comment:3 follow-up: ↓ 5 Changed 10 years ago by
comment:4 Changed 10 years ago by
- Keywords sd31 added
- Status changed from new to needs_review
comment:5 in reply to: ↑ 3 Changed 10 years ago by
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
- 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).)
comment:7 Changed 10 years ago by
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: ↓ 9 Changed 10 years ago by
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
Color hashing is already broken in terms of any possible sane equality comparison:
Oh! Interesting!
comment:10 Changed 10 years ago by
Ping - anyone doing anything with this?
comment:11 Changed 10 years ago by
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'): 364 364 # For backward compatibility. 365 365 to_mpl_color = rgbcolor 366 366 367 from sage.structure.unique_representation import UniqueRepresentation 367 368 368 class Color(object): 369 class 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 369 376 def __init__(self, r='#0000ff', g=None, b=None, space='rgb'): 370 377 """ 371 378 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
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
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.
comment:14 Changed 9 years ago by
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
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
comment:16 Changed 9 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Patch is needed for #12904.
comment:17 follow-up: ↓ 19 Changed 9 years ago by
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
orsage: 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 the23
comparison inne
.- 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.
comment:18 Changed 9 years ago by
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
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: ↓ 21 Changed 9 years ago by
- 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
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
- 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
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
- 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
- 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
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)?