tulip-control / polytope

Geometric operations on polytopes of any dimension
https://pypi.org/project/polytope
Other
74 stars 19 forks source link

Error message says "Cannot plot polytopes of dimension larger than 2", but can't plot dimension 1 aswell #68

Closed TomLKoller closed 3 years ago

TomLKoller commented 3 years ago

I tried to plot 1D polytopes and i got quite confused when it told me that it cant plot with dimension larger than 2. This formulation implies, that it can plot 1D polytopes.

I suggest to change to

"Cannot plot polytopes of dimension other than 2"

johnyf commented 3 years ago

Thank you for reporting this. The Exception is raised at:

https://github.com/tulip-control/polytope/blob/f981f434a270ee005076b9c534383aeaba314f40/polytope/polytope.py#L404-L405

Following the changes of the code in the past, the test of dimension != 2, and the message have been essentially unchanged since this code was introduced (the message has changed by removing a whitespace character), in tulip: https://github.com/tulip-control/tulip-control/commit/85a9ef325b7391198c1201b2aefd9e7db59ca50b#diff-a3402e018e75772f98fab2fad2a90be9bb565a04576ec077b6e5f5aebb5d89beR94-R96

Initially, this check printed a message and returned early from the call. Later, printing changed to logging, and logging to raising an exception (this change was in 8407888458da0e492bd512c256ecba3a64e77e29).

Regarding possibilities for change, the existing implementation of the function polytope.polytope._get_patch passes the polytope vertices to the constructor of the class matplotlib.patches.Polygon:

https://github.com/tulip-control/polytope/blob/f981f434a270ee005076b9c534383aeaba314f40/polytope/polytope.py#L2205

The docstring of the class matplotlib.patches.Polygon requires two-dimensional points:

https://github.com/matplotlib/matplotlib/blob/cc1f776fd109090d833cb4250b56665a3746c55f/lib/matplotlib/patches.py#L1074

So with the current implementation, plotting one-dimensional polytopes is not supported. If this is ever implemented, some convention would be needed, e.g., that one-dimensional polytopes are plotted on the x-axis. Until then, the proposed change of replacing the word "larger" with "other" appears to be a good choice.

This issue applies also to the analogous test and message about regions (a class that can represent polytopes that can be nonconvex):

https://github.com/tulip-control/polytope/blob/f981f434a270ee005076b9c534383aeaba314f40/polytope/polytope.py#L867-L868

johnyf commented 3 years ago

Addressed in commit ed6e79812569b921ccfe305ca0c92a352bbe3b4b, which is now included in the mainline branch.