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

Fix Polyhedron.base_extend so it does not ignore the backend parameter #22575

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

Fix Polyhedron.base_extend so it does not ignore the backend parameter #22575

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

Comments

@mforets
Copy link
Mannequin

mforets mannequin commented Mar 10, 2017

Polyhedra can be instantiated with different backends (CDD, PPL, normaliz, ...), and the base_extend method is supposed to allow to transform between backends, but the parameter is simply ignored (in (Polyhedra_base.base_extend).

Possible follow-up: Also a new method change_backend could be implemented as an alias. (Once done, add it to tutorial, too.)

CC: @mkoeppe @jplab @tscrim

Component: geometry

Keywords: days84, polytope

Author: Matthias Koeppe

Branch/Commit: 9f61dac

Reviewer: Travis Scrimshaw

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

@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

base_extend should do the job. But actually... the optional parameter to change the backend is simply not used at all!?!?!?!

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_cdd = P.base_extend(base_ring=ZZ,backend='cdd')
sage: type(P_cdd)
<class 'sage.geometry.polyhedron.backend_ppl.Polyhedra_ZZ_ppl_with_category.element_class'>

This is because in the parent class, the method .base_extend() of parent does not even use the optional parameter backend...

sage: par = P.parent()
sage: par.base_extend??     # this is worth looking at...

I guess one could simply correct that...

Now the question is whether there is a smart way to go from one to another?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 28, 2017

comment:2

See also #22701.

@mforets
Copy link
Mannequin Author

mforets mannequin commented Apr 11, 2017

comment:3

Replying to @jplab:

...
This is because in the parent class, the method .base_extend() of parent does not even use the optional parameter backend...

sage: par = P.parent()
sage: par.base_extend??     # this is worth looking at...

good finding, maybe this was a feature that didn't get in at the time.

also, i didn't find a command to get the backend as a string, like P.backend(), or P.get_backend(). it seems that there is no object (list) of available_backends either.

I guess one could simply correct that...

Now the question is whether there is a smart way to go from one to another?

i suggest to just start with the naive way and see how it behaves.

@jplab

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Add .change_backend() method for polyhedra Fix Polyhedron.base_extend so it does not ignore the backend parameter Sep 29, 2018
@mkoeppe mkoeppe modified the milestones: sage-7.6, sage-8.4 Sep 29, 2018
@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2018

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2018

Commit: 260739d

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2018

New commits:

260739d{Polyhedron_base, Polyhedron_base}.base_extend: Handle backend.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2018

Author: Matthias Koeppe

@tscrim
Copy link
Collaborator

tscrim commented Sep 30, 2018

comment:8

So my only quip is that this defaults to the automatic backend choice when the backend is not specified. I would expect that the backend be the same backend as what I am currently working with:

sage: P = Polyhedron(vertices=[(1,0), (0,1)], rays=[(1,1)], base_ring=QQ)
sage: P.backend()
'ppl'
sage: P = Polyhedron(vertices=[(1,0), (0,1)], rays=[(1,1)], base_ring=ZZ, backend='normaliz')
sage: P.backend()
'normaliz'
sage: P.base_extend(QQ).backend()
'ppl'

So if you think this is the behavior that it should have, then you should put a .. WARNING:: explaining that the backend is given by the default if not specified.

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 30, 2018

comment:9

Replying to @tscrim:

So my only quip is that this defaults to the automatic backend choice when the backend is not specified. I would expect that the backend be the same backend as what I am currently working with:

One might expect that, but the backend may simply not be suitable. Consider base_extend of a PPL polytope to AA - only the 'field' backend is suitable.

I'll add some documentation.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2018

Changed commit from 260739d to 9f61dac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2018

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

9f61dacPolyhedron.base_extend docstring: Explain default value of backend

@tscrim
Copy link
Collaborator

tscrim commented Sep 30, 2018

comment:11

Thanks. LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Sep 30, 2018

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Oct 1, 2018

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