Opened 6 months ago
Closed 4 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:  sage9.4 
Component:  geometry  Keywords:  richcmp, polyhedra, vrepresentation, hrepresentation 
Cc:  mkoeppe, ghkliem, 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 6 months ago by
comment:2 Changed 6 months ago by
 Branch set to u/yzh/polyhedronrepresentation_richcmp_compares_two_representation_objects_with_different_types
comment:3 Changed 6 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 6 months ago by
if you add author name and set it to "needs review", the patchbot will run it
comment:5 Changed 6 months ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:6 Changed 6 months ago by
 Status changed from needs_review to needs_work
This needs doctests that illustrate the new behavior.
comment:7 Changed 6 months ago by
 Commit changed from 2803d29d06cc7c683654cba34a2e1b724198bb11 to 7af9ebcbd58a9139145fd017fa69d3f1cbe3f996
comment:8 Changed 6 months ago by
 Status changed from needs_work to needs_review
comment:9 Changed 6 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 nonbugfix tickets.
comment:10 Changed 6 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 6 months ago by
 Status changed from needs_work to needs_review
comment:12 Changed 6 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 6 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 6 months ago by
 Status changed from needs_work to needs_review
comment:16 Changed 4 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".