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: |
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)
Change History (13)
comment:1 Changed 11 years ago by
- Milestone set to sage-4.4
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
- Status changed from needs_review to positive_review
apply, tests, doc...
Positive review for this patch.
comment:3 Changed 11 years ago by
- Reviewers set to Nicolas Borie
comment:4 follow-up: ↓ 7 Changed 11 years ago by
- 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 11 years ago by
Also, this would have been caught by a _test_eq, which we should implement! See #8697!
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 11 years ago by
comment:7 in reply to: ↑ 4 ; follow-up: ↓ 9 Changed 11 years ago by
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
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 11 years ago by
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
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 11 years ago by
- 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
- 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
- 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.
This should be ready for review.