Opened 5 years ago
Closed 5 years 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, 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 follow-up: ↓ 3 Changed 5 years ago by
comment:2 Changed 5 years ago by
- Status changed from new to needs_info
comment:3 in reply to: ↑ 1 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 set to u/moritz/22565
comment:5 Changed 5 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 5 years 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 5 years ago by
- Commit changed from e1e56a7bf9c73d8455cad526094d4d911e312abb to 739af0c819306f8efdde2772245e0dc4f7d20e18
comment:8 follow-up: ↓ 9 Changed 5 years 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 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 changed from 739af0c819306f8efdde2772245e0dc4f7d20e18 to 3bd35ca89d586baea6bc9917e46c32f2aff741e8
Branch pushed to git repo; I updated commit sha1. New commits:
3bd35ca | added normaliz and polymake doctests
|
comment:11 follow-up: ↓ 12 Changed 5 years 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 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))]) **********************************************************************
If you don't want to see the QQ(.)
, there's the option preparse=False
in sage_input
.
comment:14 Changed 5 years ago by
- Branch changed from u/moritz/22565 to u/jipilab/22565
comment:15 Changed 5 years ago by
- 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:
dd3a121 | Fixed a failing polymake test
|
comment:16 Changed 5 years ago by
- Status changed from needs_review to positive_review
thanks for the review, JP!
comment:17 Changed 5 years ago by
- Status changed from positive_review to needs_work
See the patchbot result
comment:18 Changed 5 years ago by
- Work issues set to Broke the associahedron
It seems that the creation of the associahedron is broken by this code (!?)
comment:19 follow-up: ↓ 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 in reply to: ↑ 19 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 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 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 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 5 years ago by
TODO::
should be
.. TODO::
Otherwise, looks good to me
comment:26 Changed 5 years 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 5 years ago by
- Status changed from needs_work to needs_review
comment:28 Changed 5 years ago by
- Milestone changed from sage-7.6 to sage-8.1
comment:29 Changed 5 years ago by
- 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 5 years 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?