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: 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:

Status badges

Description (last modified by mkoeppe)

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 gh-kliem

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".

comment:2 Changed 6 months ago by yzh

  • Branch set to u/yzh/polyhedronrepresentation_richcmp_compares_two_representation_objects_with_different_types

comment:3 Changed 6 months ago by mkoeppe

  • 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:

2803d29make PolyhedronRepresentation richcmp compare two representation objects with different types

comment:4 Changed 6 months ago by mkoeppe

if you add author name and set it to "needs review", the patchbot will run it

comment:5 Changed 6 months ago by yzh

  • Authors set to Yuan Zhou
  • Description modified (diff)
  • Status changed from new to needs_review

comment:6 Changed 6 months ago by mkoeppe

  • Status changed from needs_review to needs_work

This needs doctests that illustrate the new behavior.

comment:7 Changed 6 months ago by git

  • Commit changed from 2803d29d06cc7c683654cba34a2e1b724198bb11 to 7af9ebcbd58a9139145fd017fa69d3f1cbe3f996

Branch pushed to git repo; I updated commit sha1. New commits:

2d57f41add doctest for 31702
7af9ebcsolve relint: commands failed

comment:8 Changed 6 months ago by yzh

  • Status changed from needs_work to needs_review

comment:9 Changed 6 months ago by mkoeppe

  • 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 6 months ago by git

  • Commit changed from 7af9ebcbd58a9139145fd017fa69d3f1cbe3f996 to 48a8e8d00dc287f6a50ee3dcbe1c1a57f19591e7

Branch pushed to git repo; I updated commit sha1. New commits:

48a8e8dreword the doctest of richcmp

comment:11 Changed 6 months ago by yzh

  • Status changed from needs_work to needs_review

comment:12 Changed 6 months ago by mkoeppe

  • 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 git

  • Commit changed from 48a8e8d00dc287f6a50ee3dcbe1c1a57f19591e7 to 73e78e2bdd6c05957dfd808d2d4038e20bf43c00

Branch pushed to git repo; I updated commit sha1. New commits:

73e78e2improve docstring of __richcmp__

comment:14 Changed 6 months ago by yzh

  • Status changed from needs_work to needs_review

comment:15 Changed 6 months ago by mkoeppe

  • Status changed from needs_review to positive_review

LGTM.

comment:16 Changed 4 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.