Opened 4 years ago

Closed 4 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) 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 4 years ago by mantepse

However:

sage: Word([1,1], alphabet=[1,2]) != Word([1,2], alphabet=[1,2])
True

???

comment:2 Changed 4 years ago by tscrim

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:3 Changed 4 years ago by mantepse

Thank you Travis! This worked, but I must say that I hate python.

comment:4 follow-up: Changed 4 years ago by mantepse

Can you explain why it fails only when assigning to a variable?

comment:5 in reply to: ↑ 4 Changed 4 years ago by slabbe

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.

Last edited 4 years ago by slabbe (previous) (diff)

comment:6 Changed 4 years ago by slabbe

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 4 years ago by slabbe

  • Authors set to Sébastien Labbé
  • Branch set to u/slabbe/21609
  • Commit set to 6b8ac18b165e0eb74e0554eb191af103d6eb3cd3
  • Status changed from new to needs_review

New commits:

6b8ac1821609: Fixing A!=B for words char when A > B

comment:8 Changed 4 years ago by mantepse

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 4 years ago by slabbe

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 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to 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...

Last edited 4 years ago by tscrim (previous) (diff)

comment:11 Changed 4 years ago by vbraun

  • Branch changed from u/slabbe/21609 to 6b8ac18b165e0eb74e0554eb191af103d6eb3cd3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.