Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better plotting for polytopes, re-add projection_direction #16625

Closed
kcrisman opened this issue Jul 8, 2014 · 23 comments
Closed

Better plotting for polytopes, re-add projection_direction #16625

kcrisman opened this issue Jul 8, 2014 · 23 comments

Comments

@kcrisman
Copy link
Member

kcrisman commented Jul 8, 2014

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])

CC: @dimpase @vbraun @novoselt

Component: geometry

Author: Volker Braun

Branch: bdb1803

Reviewer: Andrey Novoseltsev

Issue created by migration from https://trac.sagemath.org/ticket/16625

@kcrisman kcrisman added this to the sage-6.3 milestone Jul 8, 2014
@kcrisman

This comment has been minimized.

@kcrisman
Copy link
Member Author

kcrisman commented Jul 8, 2014

comment:2

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

@kcrisman

This comment has been minimized.

@kcrisman kcrisman changed the title Schlegel doesn't work for all polytopes projection_direction broken for polytopes Jul 8, 2014
@vbraun
Copy link
Member

vbraun commented Jul 8, 2014

comment:4

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

@kcrisman
Copy link
Member Author

kcrisman commented Jul 8, 2014

comment:5

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.

@vbraun
Copy link
Member

vbraun commented Jul 9, 2014

@vbraun
Copy link
Member

vbraun commented Jul 9, 2014

Commit: fa898d1

@vbraun
Copy link
Member

vbraun commented Jul 9, 2014

New commits:

fa898d1Better plotting of polyhedra

@vbraun
Copy link
Member

vbraun commented Jul 9, 2014

Author: Volker Braun

@vbraun vbraun changed the title projection_direction broken for polytopes Better plotting for polytopes, re-add projection_direction Jul 9, 2014
@kcrisman
Copy link
Member Author

kcrisman commented Jul 9, 2014

comment:8

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2014

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

8a213a7Make projection objects display as graphics

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2014

Changed commit from fa898d1 to 8a213a7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2014

Changed commit from 8a213a7 to cf41e5b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2014

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

cf41e5bFix deprecations and doctests

@vbraun
Copy link
Member

vbraun commented Jul 16, 2014

comment:11

anybody feels like reviewing this?

@novoselt
Copy link
Member

comment:12

Working on it.

@novoselt
Copy link
Member

Reviewer: Andrey Novoseltsev

@novoselt
Copy link
Member

@novoselt
Copy link
Member

comment:14

lgtm and works fine!


New commits:

bdb1803Reorder arguments description.

@novoselt
Copy link
Member

Changed commit from cf41e5b to bdb1803

@vbraun
Copy link
Member

vbraun commented Jul 17, 2014

Changed branch from u/novoselt/projection_direction_broken_for_polytopes to bdb1803

@kcrisman
Copy link
Member Author

kcrisman commented Nov 4, 2014

comment:16

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.

@kcrisman
Copy link
Member Author

kcrisman commented Nov 4, 2014

Changed commit from bdb1803 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants