Opened 6 years ago
Closed 5 years ago
#22565 closed defect (fixed)
Make "sage_input" for polyhedron objects output the backend used
Reported by:  JeanPhilippe Labbé  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  geometry  Keywords:  days84, geometry 
Cc:  Moritz Firsching, Matthias Köppe, Vincent Delecroix, Thierry Monteil, Frédéric Chapoton  Merged in:  
Authors:  Moritz Firsching  Reviewers:  JeanPhilippe Labbé, Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  Broke the associahedron 
Branch:  28e2b71 (Commits, GitHub, GitLab)  Commit:  28e2b71ef1a9dbe0725e873eec16662f6db593dc 
Dependencies:  Stopgaps: 
Description
Currently, the sage_input
function of a Polyhedron object does not provide the backend used.
sage: P1 = Polyhedron(vertices = [[1, 0], [0, 1]], rays = [[1, 1]], backend='normaliz') sage: sage_input(P1) Polyhedron(base_ring=ZZ, rays=[(1, 1)], vertices=[(0, 1), (1, 0)]) sage: P2 = Polyhedron(vertices = [[1, 0], [0, 1]], rays = [[1, 1]], backend='ppl') sage: sage_input(P2) Polyhedron(base_ring=ZZ, rays=[(1, 1)], vertices=[(0, 1), (1, 0)])
It should be able to give it.
Change History (30)
comment:1 followup: 3 Changed 6 years ago by
comment:2 Changed 5 years ago by
Status:  new → needs_info 

comment:3 Changed 5 years ago by
Replying to moritz:
I agree that we need to have this feature! Perhaps it would make sense to implement a function
get_backend
(like we have inMixedIntegerLinearProgram
), that returns thebackend
first? (Or perhaps simply call this functionbackend
?
+1 for getting a function backend
. (I would choose backend
to make it similar to base_ring
and further starting with get_
is annoying when using tab completion. Just to be sure I would look at the naming conventions of Sage, I guess backend
is better.)
So I guess it would be cleanest to store the backend information somehow in
P1._backend
. Should this be done directly in the init ofclass Polyhedra_base
inparent.py
, similar toP1._ambient_dim
? Or should this rather be done inside each backend class? Or yet elsewhere?
One way to go, since each (backend/base_ring) combination creates a different parent in parent.py would be to create an init function for each of these parents that executes the init of the Polyhedra_base
and adds an attribute _backend
on top.
Because doing it in Polyhedra_base seems to be messy (how to know the backend information at this stage?).
comment:4 Changed 5 years ago by
Branch:  → u/moritz/22565 

comment:5 Changed 5 years ago by
Commit:  → e1e56a7bf9c73d8455cad526094d4d911e312abb 

I looked at it again and now think its best to put it in parent.py.
There a treatment that is analogous to ambient_dim
and base_ring
is possible. Please have a look at the patch!
Once the backend
function is approved it will be easy to enhance the sage_input
function as requested in the description of the ticket.
New commits:
e1e56a7  introduce backend parameter, variable and function

comment:6 Changed 5 years ago by
Status:  needs_info → needs_work 

The backend method looks good. I guess you can proceed with the sage_input part.
Thanks!
comment:7 Changed 5 years ago by
Commit:  e1e56a7bf9c73d8455cad526094d4d911e312abb → 739af0c819306f8efdde2772245e0dc4f7d20e18 

comment:8 followup: 9 Changed 5 years ago by
I have not (yet) added doctests polymake and (py)normaliz; do you think this is needed?
comment:9 Changed 5 years ago by
Replying to moritz:
I have not (yet) added doctests polymake and (py)normaliz; do you think this is needed?
Yes, that would be nice! Especially since they are backends and it's about backends...
comment:10 Changed 5 years ago by
Commit:  739af0c819306f8efdde2772245e0dc4f7d20e18 → 3bd35ca89d586baea6bc9917e46c32f2aff741e8 

Branch pushed to git repo; I updated commit sha1. New commits:
3bd35ca  added normaliz and polymake doctests

comment:11 followup: 12 Changed 5 years ago by
Status:  needs_work → needs_review 

I have not tested the polymake doctest, since I don't have polymake installed at the moment. Let's see what the bot says..
comment:12 Changed 5 years ago by
I have not tested the polymake doctest, since I don't have polymake installed at the moment. Let's see what the bot says..
All right, I'll have a look!
comment:13 Changed 5 years ago by
For now, I get:
sage t base.py ********************************************************************** File "base.py", line 171, in sage.geometry.polyhedron.base.Polyhedron_base._sage_input_ Failed example: sage_input(P) # optional  polymake Expected: Polyhedron(backend='polymake', base_ring=ZZ, rays=[(1, 1)], vertices=[(0, 1), (1, 0)]) Got: Polyhedron(backend='polymake', base_ring=QQ, rays=[(QQ(1), QQ(1))], vertices=[(QQ(1), QQ(0)), (QQ(0), QQ(1))]) **********************************************************************
So there's a conversion in polymake
to QQ
?
comment:14 Changed 5 years ago by
Branch:  u/moritz/22565 → u/jipilab/22565 

comment:15 Changed 5 years ago by
Authors:  → Moritz Firsching 

Commit:  3bd35ca89d586baea6bc9917e46c32f2aff741e8 → dd3a1210c6d597995c2483ffec66c4d881ccce8e 
Reviewers:  → JeanPhilippe Labbé 
Hi Moritz,
All tests pass on beta3. It seems to fix the issue with the backend. I put a TODO in the doctest to add handling of the preparse
option of sage_input
eventually to get rid of the QQ
in the output.
Look at the change and if you are happy, you can set it as positive review.
New commits:
dd3a121  Fixed a failing polymake test

comment:18 Changed 5 years ago by
Work issues:  → Broke the associahedron 

It seems that the creation of the associahedron is broken by this code (!?)
comment:19 followup: 20 Changed 5 years ago by
Maybe the backend must be specified in
associahedron = super(Associahedra, self)._element_constructor_(None, [inequalities, []])
near the end of src/sage/combinat/root_system/associahedron.py
comment:20 Changed 5 years ago by
Maybe the backend must be specified in
associahedron = super(Associahedra, self)._element_constructor_(None, [inequalities, []])near the end of src/sage/combinat/root_system/associahedron.py
That does not seem to be enough. I'm puzzled by how to fix this. Now the __init__
of the polyhedron base class requires 4 arguments. But how to pass this new argument when having a subparent class Associahedra
.
I changed also line 33 like this:
33 parent = Associahedra(QQ, cartan_type.rank(),'ppl')
to get the correct backend. But I still get similar errors. Any cues?
comment:21 Changed 5 years ago by
Branch:  u/jipilab/22565 → public/22565 

Commit:  dd3a1210c6d597995c2483ffec66c4d881ccce8e → 951735b5428187980dfa798b7f52fe293733494d 
Here is a tentative that works.. Maybe Travis would find a better way ?
New commits:
951735b  trac 22565 fixing associahedra (sort of)

comment:22 Changed 5 years ago by
Oh, I see! But isn't this a bit weird considering the fact that the class Associahedra
inherits from Polyhedra_QQ_ppl
?
comment:23 Changed 5 years ago by
I think it is a bad idea in the first place to have a class such as Associahedron_class
inherit from a particular polyhedron backend class.
comment:24 Changed 5 years ago by
The bot seems to like the hack.
Perhaps as a followup, it would be good to change the inheritance of the Associahedron_class
to Polyhedra
? What do you think?
Otherwise, as of now, I would put this ticket as positive review.
comment:26 Changed 5 years ago by
Commit:  951735b5428187980dfa798b7f52fe293733494d → 28e2b71ef1a9dbe0725e873eec16662f6db593dc 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
79af7fc  introduce backend parameter, variable and function

a8de4a7  backend function in base.py

deebcf7  added 'backend' to 'sage_input'

c10357f  added normaliz and polymake doctests

435525a  Fixed a failing polymake test

04eda4d  trac 22565 fixing associahedra (sort of)

28e2b71  rebase and '.. TODO'

comment:27 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:28 Changed 5 years ago by
Milestone:  sage7.6 → sage8.1 

comment:29 Changed 5 years ago by
Reviewers:  JeanPhilippe Labbé → JeanPhilippe Labbé, Frédéric Chapoton 

Status:  needs_review → positive_review 
This looks good to go. The last bot gave a green light.
comment:30 Changed 5 years ago by
Branch:  public/22565 → 28e2b71ef1a9dbe0725e873eec16662f6db593dc 

Resolution:  → fixed 
Status:  positive_review → closed 
I agree that we need to have this feature! Perhaps it would make sense to implement a function
get_backend
(like we have inMixedIntegerLinearProgram
), that returns thebackend
first? (Or perhaps simply call this functionbackend
?So I guess it would be cleanest to store the backend information somehow in
P1._backend
. Should this be done directly in the init ofclass Polyhedra_base
inparent.py
, similar toP1._ambient_dim
? Or should this rather be done inside each backend class? Or yet elsewhere?What do you think?