Opened 2 years ago

Closed 18 months ago

#22565 closed defect (fixed)

Make "sage_input" for polyhedron objects output the backend used

Reported by: jipilab Owned by:
Priority: major Milestone: sage-8.1
Component: geometry Keywords: days84, geometry
Cc: moritz, mkoeppe, vdelecroix, tmonteil, chapoton Merged in:
Authors: Moritz Firsching Reviewers: Jean-Philippe 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 follow-up: Changed 23 months ago by moritz

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?

comment:2 Changed 21 months ago by moritz

  • Status changed from new to needs_info

comment:3 in reply to: ↑ 1 Changed 21 months ago by jipilab

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

comment:4 Changed 20 months ago by moritz

  • Branch set to u/moritz/22565

comment:5 Changed 20 months ago by moritz

  • 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:

e1e56a7introduce backend parameter, variable and function

comment:6 Changed 19 months ago by jipilab

  • 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 19 months ago by git

  • Commit changed from e1e56a7bf9c73d8455cad526094d4d911e312abb to 739af0c819306f8efdde2772245e0dc4f7d20e18

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

afa5286introduce backend parameter, variable and function
e7f81ecbackend function in base.py
739af0cadded 'backend' to 'sage_input'

comment:8 follow-up: Changed 19 months ago by moritz

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

comment:9 in reply to: ↑ 8 Changed 19 months ago by jipilab

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 19 months ago by git

  • Commit changed from 739af0c819306f8efdde2772245e0dc4f7d20e18 to 3bd35ca89d586baea6bc9917e46c32f2aff741e8

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

3bd35caadded normaliz and polymake doctests

comment:11 follow-up: Changed 19 months ago by moritz

  • 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 19 months ago by jipilab

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 19 months ago by jipilab

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.

Last edited 19 months ago by jipilab (previous) (diff)

comment:14 Changed 19 months ago by jipilab

  • Branch changed from u/moritz/22565 to u/jipilab/22565

comment:15 Changed 19 months ago by jipilab

  • Authors set to Moritz Firsching
  • Commit changed from 3bd35ca89d586baea6bc9917e46c32f2aff741e8 to dd3a1210c6d597995c2483ffec66c4d881ccce8e
  • Reviewers set to Jean-Philippe 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:

dd3a121Fixed a failing polymake test

comment:16 Changed 19 months ago by moritz

  • Status changed from needs_review to positive_review

thanks for the review, JP!

comment:17 Changed 19 months ago by vbraun

  • Status changed from positive_review to needs_work

See the patchbot result

comment:18 Changed 19 months ago by jipilab

  • Work issues set to Broke the associahedron

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

comment:19 follow-up: Changed 19 months ago by chapoton

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 19 months ago by jipilab

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 19 months ago by chapoton

  • 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:

951735btrac 22565 fixing associahedra (sort of)

comment:22 Changed 19 months ago by jipilab

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 19 months ago by mkoeppe

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 19 months ago by jipilab

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.

comment:25 Changed 19 months ago by chapoton

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

comment:26 Changed 19 months ago by git

  • Commit changed from 951735b5428187980dfa798b7f52fe293733494d to 28e2b71ef1a9dbe0725e873eec16662f6db593dc

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

79af7fcintroduce backend parameter, variable and function
a8de4a7backend function in base.py
deebcf7added 'backend' to 'sage_input'
c10357fadded normaliz and polymake doctests
435525aFixed a failing polymake test
04eda4dtrac 22565 fixing associahedra (sort of)
28e2b71rebase and '.. TODO'

comment:27 Changed 19 months ago by moritz

  • Status changed from needs_work to needs_review

comment:28 Changed 19 months ago by moritz

  • Milestone changed from sage-7.6 to sage-8.1

comment:29 Changed 18 months ago by jipilab

  • Reviewers changed from Jean-Philippe Labbé to Jean-Philippe 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 18 months ago by vbraun

  • Branch changed from public/22565 to 28e2b71ef1a9dbe0725e873eec16662f6db593dc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.