Make "sage_input" for polyhedron objects output the backend used

[jplab]

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.

CC: @mo271 @mkoeppe @videlec @sagetrac-tmonteil @fchapoton

Component: geometry

Keywords: days84

Work Issues: Broke the associahedron

Author: Moritz Firsching

Branch/Commit: 28e2b71

Reviewer: Jean-Philippe Labbé, Frédéric Chapoton

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

[mo271]

comment:1

I agree that we need to have this feature! Perhaps it would make sense to implement a function get_backend (like we have in MixedIntegerLinearProgram), that returns the backend first? (Or perhaps simply call this function backend?

So I guess it would be cleanest to store the backend information somehow in P1._backend. Should this be done directly in the init of class Polyhedra_base in parent.py, similar to P1._ambient_dim? Or should this rather be done inside each backend class? Or yet elsewhere?

What do you think?

[jplab]

comment:3

Replying to @mo271:

I agree that we need to have this feature! Perhaps it would make sense to implement a function get_backend (like we have in MixedIntegerLinearProgram), that returns the backend first? (Or perhaps simply call this function backend?

+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 of class Polyhedra_base in parent.py, similar to P1._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?).

[mo271]

Branch: u/moritz/22565

[mo271]

comment:5

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

[mo271]

Commit: e1e56a7

[jplab]

comment:6

The backend method looks good. I guess you can proceed with the sage_input part.

Thanks!

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

afa5286	introduce backend parameter, variable and function
e7f81ec	backend function in base.py
739af0c	added 'backend' to 'sage_input'

[sagetrac-git]

Changed commit from e1e56a7 to 739af0c

[mo271]

comment:8

I have not (yet) added doctests polymake and (py)normaliz; do you think this is needed?

[jplab]

comment:9

Replying to @mo271:

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...

[sagetrac-git]

Changed commit from 739af0c to 3bd35ca

[sagetrac-git]

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

3bd35ca

added normaliz and polymake doctests

[mo271]

comment:11

I have not tested the polymake doctest, since I don't have polymake installed at the moment. Let's see what the bot says..

[jplab]

comment:12

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!

[jplab]

comment:13

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.

[jplab]

Changed branch from u/moritz/22565 to u/jipilab/22565

[jplab]

Changed commit from 3bd35ca to dd3a121

[jplab]

Reviewer: Jean-Philippe Labbé

[jplab]

Author: Moritz Firsching

[jplab]

comment:15

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

[vbraun]

comment:17

See the patchbot result

[jplab]

Work Issues: Broke the associahedron

[jplab]

comment:18

It seems that the creation of the associahedron is broken by this code (!?)

[fchapoton]

comment:19

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

[jplab]

comment:20

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?

[fchapoton]

Changed branch from u/jipilab/22565 to public/22565

[fchapoton]

comment:21

Here is a tentative that works.. Maybe Travis would find a better way ?

---

New commits:

951735b

trac 22565 fixing associahedra (sort of)

[fchapoton]

Changed commit from dd3a121 to 951735b

[jplab]

comment:22

Oh, I see! But isn't this a bit weird considering the fact that the class Associahedra inherits from Polyhedra_QQ_ppl?

[mkoeppe]

comment:23

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.

[jplab]

comment:24

The bot seems to like the hack.

Perhaps as a follow-up, 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.

[fchapoton]

comment:25

TODO::
should be
.. TODO::
Otherwise, looks good to me

[sagetrac-git]

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'

[sagetrac-git]

Changed commit from 951735b to 28e2b71

[jplab]

comment:29

This looks good to go. The last bot gave a green light.

[jplab]

Changed reviewer from Jean-Philippe Labbé to Jean-Philippe Labbé, Frédéric Chapoton

[vbraun]

Changed branch from public/22565 to 28e2b71