Opened 4 years ago
Closed 3 years ago
#22575 closed enhancement (fixed)
Fix Polyhedron.base_extend so it does not ignore the backend parameter
Reported by:  mforets  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  geometry  Keywords:  days84, polytope 
Cc:  mkoeppe, jipilab, tscrim  Merged in:  
Authors:  Matthias Koeppe  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  9f61dac (Commits, GitHub, GitLab)  Commit:  9f61dac34e2f4a755fbb8659bb2f7c6ab3130b5c 
Dependencies:  Stopgaps: 
Description (last modified by )
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 followup: Also a new method change_backend
could be implemented as an alias. (Once done, add it to tutorial, too.)
Change History (12)
comment:1 followup: ↓ 3 Changed 4 years ago by
comment:2 Changed 4 years ago by
See also #22701.
comment:3 in reply to: ↑ 1 Changed 4 years ago by
Replying to jipilab:
... This is because in the parent class, the method
.base_extend()
ofparent
does not even use the optional parameterbackend
...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.
comment:4 Changed 3 years ago by
 Description modified (diff)
comment:5 Changed 3 years ago by
 Cc tscrim added
 Description modified (diff)
 Milestone changed from sage7.6 to sage8.4
 Summary changed from Add .change_backend() method for polyhedra to Fix Polyhedron.base_extend so it does not ignore the backend parameter
comment:6 Changed 3 years ago by
 Branch set to u/mkoeppe/fix_polyhedron_base_extend_so_it_does_not_ignore_the_backend_parameter
comment:7 Changed 3 years ago by
 Commit set to 260739db6ac8ee46b50db4d1489d11b0621c1c36
 Description modified (diff)
 Status changed from new to needs_review
New commits:
260739d  {Polyhedron_base, Polyhedron_base}.base_extend: Handle backend.

comment:8 followup: ↓ 9 Changed 3 years ago by
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.
comment:9 in reply to: ↑ 8 Changed 3 years ago by
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.
comment:10 Changed 3 years ago by
 Commit changed from 260739db6ac8ee46b50db4d1489d11b0621c1c36 to 9f61dac34e2f4a755fbb8659bb2f7c6ab3130b5c
Branch pushed to git repo; I updated commit sha1. New commits:
9f61dac  Polyhedron.base_extend docstring: Explain default value of backend

comment:11 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Thanks. LGTM.
comment:12 Changed 3 years ago by
 Branch changed from u/mkoeppe/fix_polyhedron_base_extend_so_it_does_not_ignore_the_backend_parameter to 9f61dac34e2f4a755fbb8659bb2f7c6ab3130b5c
 Resolution set to fixed
 Status changed from positive_review to closed
base_extend
should do the job. But actually... the optional parameter to change the backend is simply not used at all!?!?!?!This is because in the parent class, the method
.base_extend()
ofparent
does not even use the optional parameterbackend
...I guess one could simply correct that...
Now the question is whether there is a smart way to go from one to another?