Opened 2 years ago
Closed 21 months ago
#22565 closed defect (fixed)
Make "sage_input" for polyhedron objects output the backend used
Reported by:  jipilab  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  geometry  Keywords:  days84, geometry 
Cc:  moritz, mkoeppe, vdelecroix, tmonteil, 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)  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 2 years ago by
comment:2 Changed 2 years ago by
 Status changed from new to needs_info
comment:3 in reply to: ↑ 1 Changed 2 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 2 years ago by
 Branch set to u/moritz/22565
comment:5 Changed 2 years ago by
 Commit set to 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 22 months ago by
 Status changed from needs_info to needs_work
The backend method looks good. I guess you can proceed with the sage_input part.
Thanks!
comment:7 Changed 22 months ago by
 Commit changed from e1e56a7bf9c73d8455cad526094d4d911e312abb to 739af0c819306f8efdde2772245e0dc4f7d20e18
comment:8 followup: ↓ 9 Changed 22 months ago by
I have not (yet) added doctests polymake and (py)normaliz; do you think this is needed?
comment:9 in reply to: ↑ 8 Changed 22 months 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 22 months ago by
 Commit changed from 739af0c819306f8efdde2772245e0dc4f7d20e18 to 3bd35ca89d586baea6bc9917e46c32f2aff741e8
Branch pushed to git repo; I updated commit sha1. New commits:
3bd35ca  added normaliz and polymake doctests

comment:11 followup: ↓ 12 Changed 22 months ago by
 Status changed from needs_work to 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 in reply to: ↑ 11 Changed 22 months 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 22 months 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))]) **********************************************************************
If you don't want to see the QQ(.)
, there's the option preparse=False
in sage_input
.
comment:14 Changed 22 months ago by
 Branch changed from u/moritz/22565 to u/jipilab/22565
comment:15 Changed 22 months ago by
 Commit changed from 3bd35ca89d586baea6bc9917e46c32f2aff741e8 to dd3a1210c6d597995c2483ffec66c4d881ccce8e
 Reviewers set to 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:16 Changed 22 months ago by
 Status changed from needs_review to positive_review
thanks for the review, JP!
comment:17 Changed 22 months ago by
 Status changed from positive_review to needs_work
See the patchbot result
comment:18 Changed 22 months ago by
 Work issues set to Broke the associahedron
It seems that the creation of the associahedron is broken by this code (!?)
comment:19 followup: ↓ 20 Changed 22 months 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 in reply to: ↑ 19 Changed 22 months 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 22 months ago by
 Branch changed from u/jipilab/22565 to public/22565
 Commit changed from dd3a1210c6d597995c2483ffec66c4d881ccce8e to 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 22 months 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 22 months 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 22 months 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:25 Changed 22 months ago by
TODO::
should be
.. TODO::
Otherwise, looks good to me
comment:26 Changed 22 months ago by
 Commit changed from 951735b5428187980dfa798b7f52fe293733494d to 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 22 months ago by
 Status changed from needs_work to needs_review
comment:28 Changed 22 months ago by
 Milestone changed from sage7.6 to sage8.1
comment:29 Changed 21 months ago by
 Reviewers changed from JeanPhilippe Labbé to JeanPhilippe Labbé, Frédéric Chapoton
 Status changed from needs_review to positive_review
This looks good to go. The last bot gave a green light.
comment:30 Changed 21 months ago by
 Branch changed from public/22565 to 28e2b71ef1a9dbe0725e873eec16662f6db593dc
 Resolution set to fixed
 Status changed from positive_review to 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?