Opened 11 years ago

Closed 11 years ago

#8695 closed defect (fixed)

UniqueRepresentations implements __eq__ but not __ne__

Reported by: hivert Owned by: hivert
Priority: major Milestone: sage-4.4
Component: misc Keywords: UniqueRepresentation, equality
Cc: sage-combinat Merged in: sage-4.4.alpha1
Authors: Florent Hivert Reviewers: Nicolas Borie
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Python manuals says:

There are no implied relationships among the comparison operators. The truth of x==y does not imply that x!=y is false. Accordingly, when defining eq(), one should also define ne() so that the operators will behave as expected.

UniqueRepresentation fails to comply with this. As a consequence:

sage: G6 = GL(6, QQ)
sage: G6 == G6
True
sage: G6 != G6
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
...
NotImplementedError: Matrix group over Rational Field not implemented.

Attachments (1)

trac_8695-uniquerep_missing__ne__-fh.patch (2.2 KB) - added by hivert 11 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by hivert

  • Milestone set to sage-4.4
  • Status changed from new to needs_review

This should be ready for review.

comment:2 Changed 11 years ago by nborie

  • Status changed from needs_review to positive_review

apply, tests, doc...

Positive review for this patch.

comment:3 Changed 11 years ago by nborie

  • Reviewers set to Nicolas Borie

comment:4 follow-up: Changed 11 years ago by nthiery

  • Cc sage-combinat added; combinat removed
  • Status changed from positive_review to needs_work

I am just not sure about forcing UniqueRepresentation? to inherit from object. Let's discuss this over the phone.

comment:5 follow-up: Changed 11 years ago by nthiery

Also, this would have been caught by a _test_eq, which we should implement! See #8697!

comment:6 in reply to: ↑ 5 ; follow-up: Changed 11 years ago by hivert

Replying to nthiery:

Also, this would have been caught by a _test_eq, which we should implement! See #8697!

Actually that exactly how I caught it except that it was with _test_self_equal which is implemented in #8402. I think #8697 should be closed as a duplicate of #8402.

comment:7 in reply to: ↑ 4 ; follow-up: Changed 11 years ago by mhansen

Replying to nthiery:

I am just not sure about forcing UniqueRepresentation? to inherit from object. Let's discuss this over the phone.

The only thing that inheriting from object does is make it a "new-style" class in Python, which is what everything should be now. In the 3.x series, old-style classes are removed. http://docs.python.org/reference/datamodel.html

comment:8 in reply to: ↑ 6 Changed 11 years ago by nthiery

Replying to hivert:

Replying to nthiery:

Also, this would have been caught by a _test_eq, which we should implement! See #8697!

Actually that exactly how I caught it except that it was with _test_self_equal which is implemented in #8402. I think #8697 should be closed as a duplicate of #8402.

Oops, right. I looked for that one and somehow missed it.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 11 years ago by nthiery

Replying to mhansen:

Replying to nthiery:

I am just not sure about forcing UniqueRepresentation? to inherit from object. Let's discuss this over the phone.

The only thing that inheriting from object does is make it a "new-style" class in Python, which is what everything should be now. In the 3.x series, old-style classes are removed. http://docs.python.org/reference/datamodel.html

Yup! So there is particularly no point about forcing it explicitly, since it will be automatically the case later, and in the mean time there is no reason to fix something that ain't broken.

We discussed with Florent over the phone. He will remove the inheritance from object, reupload the patch, and set a positive review on my behalf.

Changed 11 years ago by hivert

comment:10 in reply to: ↑ 9 ; follow-up: Changed 11 years ago by hivert

  • Status changed from needs_work to needs_review

We discussed with Florent over the phone. He will remove the inheritance from object, reupload the patch, and set a positive review on my behalf.

Done !

comment:11 in reply to: ↑ 10 Changed 11 years ago by hivert

  • Status changed from needs_review to positive_review

Replying to hivert:

We discussed with Florent over the phone. He will remove the inheritance from object, reupload the patch, and set a positive review on my behalf.

Done !

At put back to positive review.

comment:12 Changed 11 years ago by jhpalmieri

  • Merged in set to sage-4.4.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged "trac_8695-uniquerep_missingne-fh.patch" into 4.4.alpha1.

Note: See TracTickets for help on using tickets.