Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16625 closed defect (fixed)

Better plotting for polytopes, re-add projection_direction

Reported by: kcrisman Owned by:
Priority: major Milestone: sage-6.3
Component: geometry Keywords:
Cc: dimpase, vbraun, novoselt Merged in:
Authors: Volker Braun Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: bdb1803 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by kcrisman)

From this ask.sagemath question, the two following should be different, but aren't.

poly = polytopes.twenty_four_cell()
poly.show()

poly.show(projection_direction=[2,5,11,17])

Change History (16)

comment:1 Changed 7 years ago by kcrisman

  • Description modified (diff)

comment:2 Changed 7 years ago by kcrisman

Even P8prime = P8.base_extend(QQ) doesn't seem to have the schlegel method, which is even more perplexing.

Last edited 7 years ago by kcrisman (previous) (diff)

comment:3 Changed 7 years ago by kcrisman

  • Description modified (diff)
  • Summary changed from Schlegel doesn't work for all polytopes to projection_direction broken for polytopes

comment:4 follow-up: Changed 7 years ago by vbraun

Polyhedra have a schlegel_projection method, and not a schlegel method. The internally used Projection class has a schlegel method.

comment:5 in reply to: ↑ 4 Changed 7 years ago by kcrisman

Polyhedra have a schlegel_projection method, and not a schlegel method. The internally used Projection class has a schlegel method.

Yes, there was some confusion in my mind earlier...

As to your comment on the ask.sagemath question, unfortunately the following is still in the documentation.

sage: poly = polytopes.twenty_four_cell()
sage: poly
A 4-dimensional polyhedron in QQ^4 defined as the convex hull of 24 vertices
sage: poly.show()
sage: poly.show(projection_direction=[2,5,11,17])

So if that is not supposed to be used in that way, it was missed whenever those methods changed. There is also no deprecation information given.

comment:6 Changed 7 years ago by vbraun

  • Branch set to u/vbraun/projection_direction_broken_for_polytopes

comment:7 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to fa898d1c2761a9b0cb180d890eb490034daad09b
  • Status changed from new to needs_review
  • Summary changed from projection_direction broken for polytopes to Better plotting for polytopes, re-add projection_direction

New commits:

fa898d1Better plotting of polyhedra

comment:8 Changed 7 years ago by kcrisman

Wow, that is a lot more than need to fix this :-) It's gone beyond what I can comfortably review in a few minutes, but overall the structure, deprecations, and doc look like a big improvement.

comment:9 Changed 7 years ago by git

  • Commit changed from fa898d1c2761a9b0cb180d890eb490034daad09b to 8a213a725a158e44ffb12bb1dffd5b6bf43f6514

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

8a213a7Make projection objects display as graphics

comment:10 Changed 7 years ago by git

  • Commit changed from 8a213a725a158e44ffb12bb1dffd5b6bf43f6514 to cf41e5b4608202dfe4fca529322f73c64738d0b5

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

cf41e5bFix deprecations and doctests

comment:11 Changed 7 years ago by vbraun

anybody feels like reviewing this?

comment:12 Changed 7 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

Working on it.

comment:13 Changed 7 years ago by novoselt

  • Branch changed from u/vbraun/projection_direction_broken_for_polytopes to u/novoselt/projection_direction_broken_for_polytopes

comment:14 Changed 7 years ago by novoselt

  • Commit changed from cf41e5b4608202dfe4fca529322f73c64738d0b5 to bdb1803badb326172dd310a79033ecbdb49904b6
  • Status changed from needs_review to positive_review

lgtm and works fine!


New commits:

bdb1803Reorder arguments description.

comment:15 Changed 7 years ago by vbraun

  • Branch changed from u/novoselt/projection_direction_broken_for_polytopes to bdb1803badb326172dd310a79033ecbdb49904b6
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:16 Changed 7 years ago by kcrisman

  • Commit bdb1803badb326172dd310a79033ecbdb49904b6 deleted

Just FYI, there was no doctesting of

+    def show(self, *args, **kwds):
+        from sage.misc.superseded import deprecation
+        deprecation(16625, 'use Projection.plot instead')
+        return self.plot(*args, **kwds)

followup will be part of #17247.

Note: See TracTickets for help on using tickets.