Opened 13 months ago
Closed 11 months ago
#31702 closed enhancement (fixed)
Extend PolyhedronRepresentation richcmp to a linear order, defining comparisons of representation objects of different types
Reported by: | yzh | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.4 |
Component: | geometry | Keywords: | richcmp, polyhedra, vrepresentation, hrepresentation |
Cc: | mkoeppe, gh-kliem, jipilab | Merged in: | |
Authors: | Yuan Zhou | Reviewers: | Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | 73e78e2 (Commits, GitHub, GitLab) | Commit: | 73e78e2bdd6c05957dfd808d2d4038e20bf43c00 |
Dependencies: | Stopgaps: |
Description (last modified by )
Currently, the comparison is implemented only for two objects of the same type (vertex/ray/line or inequality/equation), as follows
if type(self) != type(other): return NotImplemented return richcmp(self._vector*self._comparison_scalar(), other._vector*other._comparison_scalar(), op)
It might be good to extend the comparison to objects of different types, using the numeric values for the output of the type() method:
INEQUALITY = 0
; EQUATION = 1
; VERTEX = 2
; RAY = 3
; LINE = 4
.
That is, instead of return NotImplemented
, we return something like
richcmp((self.type(), self._vector*self._comparison_scalar()), (other.type(), other._vector*other._comparison_scalar()), op)
In this way, we will be able to sort a list of polyhedra according to their Vrepresentations.
sage: P = Polyhedron(vertices=[(0,0),(1,1)]) sage: Q = Polyhedron(vertices=[(0,0)], rays=[(1,1)]) sage: sorted([Q, P], key=lambda p: p.Vrepresentation()) Traceback (most recent call ... TypeError: '<' not supported between instances of 'Ray' and 'Vertex'
Change History (16)
comment:1 Changed 13 months ago by
comment:2 Changed 13 months ago by
- Branch set to u/yzh/polyhedronrepresentation_richcmp_compares_two_representation_objects_with_different_types
comment:3 Changed 13 months ago by
- Commit set to 2803d29d06cc7c683654cba34a2e1b724198bb11
- Summary changed from PolyhedronRepresentation richcmp compares two representation objects with different types to Extend PolyhedronRepresentation richcmp to a linear order, defining comparisons of representation objects of different types
New commits:
2803d29 | make PolyhedronRepresentation richcmp compare two representation objects with different types
|
comment:4 Changed 13 months ago by
if you add author name and set it to "needs review", the patchbot will run it
comment:5 Changed 13 months ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:6 Changed 13 months ago by
- Status changed from needs_review to needs_work
This needs doctests that illustrate the new behavior.
comment:7 Changed 13 months ago by
- Commit changed from 2803d29d06cc7c683654cba34a2e1b724198bb11 to 7af9ebcbd58a9139145fd017fa69d3f1cbe3f996
comment:8 Changed 13 months ago by
- Status changed from needs_work to needs_review
comment:9 Changed 13 months ago by
- Status changed from needs_review to needs_work
The wording "Check :trac:31702
" suggests that this is a test for a bug -- but this ticket adds a feature, rather than fixing a bug.
It would be better to reword it so that it becomes part of the documentation of the sorting order. There is no need to refer to a ticket number for non-bugfix tickets.
comment:10 Changed 13 months ago by
- Commit changed from 7af9ebcbd58a9139145fd017fa69d3f1cbe3f996 to 48a8e8d00dc287f6a50ee3dcbe1c1a57f19591e7
Branch pushed to git repo; I updated commit sha1. New commits:
48a8e8d | reword the doctest of richcmp
|
comment:11 Changed 13 months ago by
- Status changed from needs_work to needs_review
comment:12 Changed 13 months ago by
- Description modified (diff)
- Reviewers set to Matthias Koeppe
- Status changed from needs_review to needs_work
The documentation only explains when two objects are equal.
comment:13 Changed 13 months ago by
- Commit changed from 48a8e8d00dc287f6a50ee3dcbe1c1a57f19591e7 to 73e78e2bdd6c05957dfd808d2d4038e20bf43c00
Branch pushed to git repo; I updated commit sha1. New commits:
73e78e2 | improve docstring of __richcmp__
|
comment:14 Changed 13 months ago by
- Status changed from needs_work to needs_review
comment:16 Changed 11 months ago by
- Branch changed from u/yzh/polyhedronrepresentation_richcmp_compares_two_representation_objects_with_different_types to 73e78e2bdd6c05957dfd808d2d4038e20bf43c00
- Resolution set to fixed
- Status changed from positive_review to closed
As long as you don't break #30954, it should be fine.
It is somehow inherited from vectors, which does lexicographic comparison. Indeed you can make this comparison consistent by using rich comparison on the types, if the types differ. I don't need it, but I don't mind it either.
I would consider it to be bad practice to rely on an
NotImplementedError
, because it is somehow a deprecation message by itself implicitly telling you "this might change in the future".