Ticket #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 | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Nicolas Borie |
| Authors: | Florent Hivert | Merged in: | sage-4.4.alpha1 |
| Dependencies: | Stopgaps: |
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
-
trac_8695-uniquerep_missing__ne__-fh.patch
(2.2 KB) -
added by hivert 3 years ago.
Change History
comment:1 Changed 3 years ago by hivert
- Status changed from new to needs_review
- Milestone set to sage-4.4
comment:2 Changed 3 years ago by nborie
- Status changed from needs_review to positive_review
apply, tests, doc...
Positive review for this patch.
comment:4 follow-up: ↓ 7 Changed 3 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: ↓ 6 Changed 3 years ago by nthiery
Also, this would have been caught by a _test_eq, which we should implement! See #8697!
comment:7 in reply to: ↑ 4 ; follow-up: ↓ 9 Changed 3 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 3 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: ↓ 10 Changed 3 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.
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 3 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 3 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 3 years ago by jhpalmieri
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.4.alpha1
Merged "trac_8695-uniquerep_missingne-fh.patch" into 4.4.alpha1.
This should be ready for review.