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

Add .change_ring() method for polyhedra #22574

Closed
mforets mannequin opened this issue Mar 10, 2017 · 16 comments
Closed

Add .change_ring() method for polyhedra #22574

mforets mannequin opened this issue Mar 10, 2017 · 16 comments

Comments

@mforets
Copy link
Mannequin

mforets mannequin commented Mar 10, 2017

Polyhedra can be defined in different rings, and this method allows to transform between rings (compare to the similar feature for matrices).

TODO:

  • Once done, add it to tutorial

CC: @mkoeppe @jplab

Component: geometry

Keywords: days84, polytope

Author: Laith Rastanawi

Branch/Commit: c9b92db

Reviewer: Jean-Philippe Labbé

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

@mforets mforets mannequin added this to the sage-7.6 milestone Mar 10, 2017
@mforets mforets mannequin added c: geometry labels Mar 10, 2017
@jplab
Copy link

jplab commented Mar 15, 2017

comment:1

There is currently:


sage: P = Polyhedron(vertices=[(1,0), (0,1)], rays=[(1,1)], base_ring=ZZ);  P
A 2-dimensional polyhedron in ZZ^2 defined as the convex hull of 2 vertices and 1 ray
sage: P_QQ = P.base_extend(QQ); P_QQ
A 2-dimensional polyhedron in QQ^2 defined as the convex hull of 2 vertices and 1 ray

Could there be a boolean parameter such as inplace (similar as in graphs and simplicial complexes...) to determine if one should create a new object or just change the base ring?

I am wondering how much changing the base ring changes the internals of the polyhedron object. Would many things break? We can try and see...

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 15, 2017

comment:2

I think there is no value in trying to implement inplace -- the various base rings are handled by different classes after all. Moreover, right now polyhedra are immutable - and it would be strange to be able to mutate the base ring but nothing else.

@jplab
Copy link

jplab commented Mar 15, 2017

comment:3

Replying to @mkoeppe:

I think there is no value in trying to implement inplace -- the various base rings are handled by different classes after all. Moreover, right now polyhedra are immutable - and it would be strange to be able to mutate the base ring but nothing else.

Agreed.

Further, I don't really like the fact that hidden in base_extend is the possibility to change the backend, which as a matter of fact doesn't work. This is not user-friendly to say the least. I would therefore also deprecate the keyword completely and point to a .change_backend() method (see #22575). I guess that such a method will take care to handle the modifications of base ring if need be...

@jplab

This comment has been minimized.

@LaisRast
Copy link

Branch: u/gh-LaisRast/change_ring

@LaisRast
Copy link

New commits:

5f61767add change_ring to base.py and parent.py

@LaisRast
Copy link

Commit: 5f61767

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 15, 2019

Changed commit from 5f61767 to 6cdd9d5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 15, 2019

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

6cdd9d5add documentation

@jplab
Copy link

jplab commented Feb 22, 2019

comment:8

Hi,

Here are a couple of things I saw:

  • Adapt the convention for the input, see for example vertex_facet_graph:
+        - ``backend`` -- the new backend, see
+            :func:`~sage.geometry.polyhedron.constructor.Polyhedron`.
+            If ``None`` (the default), use the same defaulting behavior
+            as described there; it is not attempted to keep the same
+            backend.
  • It would be good to have more examples with all the potential pairings.

  • Further, it would be nice to handle the potential error in the function when coercing from QQ to ZZ and return a message like: 'Can not change the ring to ZZ: a coordinate is rational'. And these examples should be added after the examples that do work.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2019

Changed commit from 6cdd9d5 to c9b92db

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 1, 2019

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

c9b92dbAdd more debug output/Add more documentation

@jplab
Copy link

jplab commented Mar 5, 2019

Author: Laith Rastanawi

@jplab
Copy link

jplab commented Mar 5, 2019

Reviewer: Jean-Philippe Labbé

@jplab
Copy link

jplab commented Mar 13, 2019

comment:12

This looks good to me.

@vbraun
Copy link
Member

vbraun commented Mar 14, 2019

Changed branch from u/gh-LaisRast/change_ring to c9b92db

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

4 participants