Opened 6 years ago
Closed 6 years ago
#21609 closed defect (fixed)
bug in inequality of words
Reported by: | mantepse | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | combinatorics | Keywords: | |
Cc: | abergeron, saliola, slabbe, amyglen | Merged in: | |
Authors: | Sébastien Labbé | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 6b8ac18 (Commits, GitHub, GitLab) | Commit: | 6b8ac18b165e0eb74e0554eb191af103d6eb3cd3 |
Dependencies: | Stopgaps: |
Description
Using 7.4.beta 6:
sage: shape1 = Word([1,2], alphabet=[1,2]) sage: shape2 = Word([1,1], alphabet=[1,2]) sage: shape1 == shape2 False sage: shape1 != shape2 False
Change History (11)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
It seems to be an issue with WordDatatype_char.__richcmp__
in that if the cmp
is non-zero, it needs to include 3 (which is !=
):
cdef int test = (<WordDatatype_char> self)._lexico_cmp(other) if test < 0: return op < 2 or op == 3 elif test > 0: - return op > 3 + return op >= 3 else: return op == 1 or op == 2 or op == 5
comment:4 follow-up: 5 Changed 6 years ago by
Can you explain why it fails only when assigning to a variable?
comment:5 Changed 6 years ago by
Looking at the fix suggested by Travis, I believe the problem is not the variable but what is on the left side vs right side: A != B
versus B != A
.
I guess the problem comes from the recent upgrade of cmp to richcmp for Python 3 support done in #21435.
comment:6 Changed 6 years ago by
Hmmm, looking at this further. I do not think this bug was included by #21435. It seems to come from the original implementation of WordDatatype_char
.
I can confirm this using sagecell which runs 7.3:
shape1 = Word([1,2], alphabet=[1,2]) shape2 = Word([1,1], alphabet=[1,2]) print shape1 == shape2, "should be False" print shape2 == shape1, "should be False" print shape1 != shape2, "should be True" print shape2 != shape1, "should be True" print shape2 == shape2, "should be True" print shape2 != shape2, "should be False" print version()
returns
False should be False False should be False False should be True True should be True True should be True False should be False SageMath version 7.3, Release Date: 2016-08-04
comment:7 Changed 6 years ago by
Authors: | → Sébastien Labbé |
---|---|
Branch: | → u/slabbe/21609 |
Commit: | → 6b8ac18b165e0eb74e0554eb191af103d6eb3cd3 |
Status: | new → needs_review |
New commits:
6b8ac18 | 21609: Fixing A!=B for words char when A > B
|
comment:8 Changed 6 years ago by
Thanks for fixing. I have a very minor suggestion. Wouldn't it be more readable to code
if test < 0: return op == 0 or op == 1 or op == 3 elif test > 0: return op == 3 or op == 4 or op == 5 else: return op == 1 or op == 2 or op == 5
comment:9 Changed 6 years ago by
Yes, I agree. But, a little bit less efficient if op == 5 when test >0 for instance...
I must leave now to take a train, I won't have acces to internet until monday.
comment:10 Changed 6 years ago by
Reviewers: | → Travis Scrimshaw |
---|---|
Status: | needs_review → positive_review |
I agree that it is a reasonable shortcut to sacrifice the slight bit of readability, but considering the comparison types are (arbitrarily) assigned integers, readability is already somewhat out the window...
comment:11 Changed 6 years ago by
Branch: | u/slabbe/21609 → 6b8ac18b165e0eb74e0554eb191af103d6eb3cd3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
However:
???