Opened 3 years ago
Closed 3 years ago
#27798 closed enhancement (fixed)
Add `backend` option to associahedron and flow polytope
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  geometry  Keywords:  polytopes, associahedron, flow polytopes, backend, normaliz 
Cc:  jipilab  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  JeanPhilippe Labbé, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e137199 (Commits, GitHub, GitLab)  Commit:  e13719990d626cef945852e79d8ef68d92a99554 
Dependencies:  Stopgaps: 
Description (last modified by )
The option backend
was missing for polytopes.associahedron
and polytopes.flow_polytope
.
Change History (44)
comment:1 Changed 3 years ago by
 Branch set to public/27798
 Commit set to 26430784821c585ea4ab52327ac76ca18b7351b1
comment:2 Changed 3 years ago by
 Commit changed from 26430784821c585ea4ab52327ac76ca18b7351b1 to a9b4826fd8db7a30960d203cc89a4e937c8785fc
Branch pushed to git repo; I updated commit sha1. New commits:
a9b4826  typo

comment:3 followups: ↓ 6 ↓ 27 Changed 3 years ago by
The doc should include in the description of backend input of both functions:
If ``None`` then ``backend='ppl'`` is used.
comment:4 Changed 3 years ago by
 Commit changed from a9b4826fd8db7a30960d203cc89a4e937c8785fc to 87169854f26eab8a4030efef1869a1883500b336
Branch pushed to git repo; I updated commit sha1. New commits:
8716985  corrected docstring

comment:5 Changed 3 years ago by
 Description modified (diff)
 Keywords polytopes associahedron flow polytopes backend normaliz added
 Status changed from new to needs_review
comment:6 in reply to: ↑ 3 Changed 3 years ago by
Replying to slabbe:
The doc should include in the description of backend input of both functions:
If ``None`` then ``backend='ppl'`` is used.
Thanks for remarking that.
Is the current even a good way of doing it?
Maybe I should just have 'ppl'
instead of None
(but this somehow looks more written in stone, does it)?
In this ticket I didn't want any changes to the default option.
comment:7 Changed 3 years ago by
 Status changed from needs_review to needs_work
Actually, I don't think the construction with associahedron works. The ._backend
string is correct, but the object does not have e.g. normaliz
functionality.
comment:8 Changed 3 years ago by
 Commit changed from 87169854f26eab8a4030efef1869a1883500b336 to b873156d9459e778e560d9a9d499002e29470011
Branch pushed to git repo; I updated commit sha1. New commits:
b873156  associahedron actually uses the claimed backend now

comment:9 Changed 3 years ago by
 Status changed from needs_work to needs_review
Seems to work now. Flow polytope anyway and also the associahedron. E.g. I can construct an associahedron with backend='normaliz'
and access (and use) the underlying normaliz cone.
comment:10 followup: ↓ 11 Changed 3 years ago by
Three little comments from a quick look:
Your INPUT:
block is overindented.
It is better to have newstyle classes:
class Associahedron_class_base(): +class Associahedron_class_base(object):
class Associahedra_base: +class Associahedra_base(object):
I do not see why the _poly_super
is needed. You could just look at the MRO or have the Polyhedra_*
class be a classlevel attribute (similar to Element
). Are you also sure that this doesn't work:
associahedron = super(self, Associahedra_base)._element_constructor_(None, [inequalities, []])
as my understanding is that it goes to the next class up in the MRO after Associahedra_base
, which should be the Polyhedra_*
class.
comment:11 in reply to: ↑ 10 Changed 3 years ago by
associahedron = super(self, Associahedra_base)._element_constructor_(None, [inequalities, []])
(with self and Associahedra_base interchanged) lead to
TypeError: super() argument 1 must be type, not classobj
However, this seems to be solved by inheriting from object
.
Replying to tscrim:
Three little comments from a quick look:
Your
INPUT:
block is overindented.It is better to have newstyle classes:
class Associahedron_class_base(): +class Associahedron_class_base(object):class Associahedra_base: +class Associahedra_base(object):I do not see why the
_poly_super
is needed. You could just look at the MRO or have thePolyhedra_*
class be a classlevel attribute (similar toElement
). Are you also sure that this doesn't work:associahedron = super(self, Associahedra_base)._element_constructor_(None, [inequalities, []])as my understanding is that it goes to the next class up in the MRO after
Associahedra_base
, which should be thePolyhedra_*
class.
comment:12 Changed 3 years ago by
 Commit changed from b873156d9459e778e560d9a9d499002e29470011 to 1f4bb257102026e66791f0c25547d7a8767467ac
Branch pushed to git repo; I updated commit sha1. New commits:
1f4bb25  comments by tscrim

comment:13 Changed 3 years ago by
You may want to have a look at #25183 while you are playing around with the associahedron.
comment:14 Changed 3 years ago by
 Commit changed from 1f4bb257102026e66791f0c25547d7a8767467ac to 2c812c508f0f5d7a8a88f132e32b149e4cc58788
Branch pushed to git repo; I updated commit sha1. New commits:
2c812c5  should have fixed bug in #25183

comment:15 Changed 3 years ago by
please fix the pyflakes warning:
+src/sage/combinat/root_system/associahedron.py:28: 'sage.geometry.polyhedron.base.Polyhedron_base' imported but unused
comment:16 Changed 3 years ago by
 Commit changed from 2c812c508f0f5d7a8a88f132e32b149e4cc58788 to 65b11c9de95bb2ecde90c722ca2e9781fdac7cc5
Branch pushed to git repo; I updated commit sha1. New commits:
65b11c9  removed redundant line

comment:17 followup: ↓ 18 Changed 3 years ago by
I don't really like this solution with __new__
; it is a bit too much of a hack IMO and seems like a (relatively) very slow thing to check. You also would not receive any benefits from the category framework (as you do not seem to be using the Parent.element_class
). Perhaps I am not quite understanding things. Could you explain a bit how you arrived at this?
comment:18 in reply to: ↑ 17 ; followup: ↓ 19 Changed 3 years ago by
JP asked me to also fix #25183. So I did.
When a polyhedral face of an associahedron is constructed, I need to somehow make sure that __new__
does not return a child of Associahedron_base
(when the argument cartan_type
is missing). This is the solution I came up with.
I have no clue how to use Parent.element_class
.
Replying to tscrim:
I don't really like this solution with
__new__
; it is a bit too much of a hack IMO and seems like a (relatively) very slow thing to check. You also would not receive any benefits from the category framework (as you do not seem to be using theParent.element_class
). Perhaps I am not quite understanding things. Could you explain a bit how you arrived at this?
comment:19 in reply to: ↑ 18 ; followup: ↓ 20 Changed 3 years ago by
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 3 years ago by
Replying to jipilab:
Replying to ghkliem:
JP asked me to also fix #25183. So I did.
Have a look at it... If the bug is related or not.
... and the bug located at #25183 should be fixed on #25183 if it is not related to the present one ... The associahedron has the category framework behind it, this is why I thought it might be a related thing. If not, then not.
comment:21 in reply to: ↑ 20 Changed 3 years ago by
I misunderstood. From my understanding #25183 should be fixed with redefining
__new__
or with changing the construction ofas_polyhedron
.
Either way, I think initialization of associahedron should ask for cartan_type
, as an argument. From my understanding an object should be valid upon construction.
Anyway, I could move my suggestions for #25183 to a new branch public/25183 as a suggestion for this (other) ticket. Then this (this) ticket would be cleaned up from this discussion.
Replying to jipilab:
Replying to jipilab:
Replying to ghkliem:
JP asked me to also fix #25183. So I did.
Have a look at it... If the bug is related or not.
... and the bug located at #25183 should be fixed on #25183 if it is not related to the present one ... The associahedron has the category framework behind it, this is why I thought it might be a related thing. If not, then not.
comment:22 Changed 3 years ago by
I think it is better to have that discussion on #25183 as it is a separate issue.
comment:23 Changed 3 years ago by
 Commit changed from 65b11c9de95bb2ecde90c722ca2e9781fdac7cc5 to f99ab5afb6f8f2de5a823d8dee45e5d809a99867
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f99ab5a  removed redundant import

comment:24 Changed 3 years ago by
Ok, I moved the corresponding part to #25183.
comment:25 Changed 3 years ago by
 Milestone changed from sage8.8 to sage8.9
comment:26 Changed 3 years ago by
Is there anything left to take care of?
comment:27 in reply to: ↑ 3 ; followup: ↓ 28 Changed 3 years ago by
The doc should include in the description of backend input of both functions:
If ``None`` then ``backend='ppl'`` is used.
Add the period at the end of the sentence.
Why is the only allowed base ring QQ
? Am I missing something?
comment:28 in reply to: ↑ 27 ; followup: ↓ 29 Changed 3 years ago by
Replying to jipilab:
The doc should include in the description of backend input of both functions:
If ``None`` then ``backend='ppl'`` is used.Add the period at the end of the sentence.
A semicolon would probably fit better. I learnt that INPUT does not stop with period.
Why is the only allowed base ring
As for the flow polytope, one can add an argument base_ring
later.
As for the Associahedron, one would need one class for each combination of base_ring
and backend
(for each child of Polyhedron_base
).
Is it worth it?
I don't see a great need for a different base ring.
(Contrary, constructing an associahedron with normaliz right away, saves a lot of time.)
Anyway, the goal of this ticket was to add a backend option. So I never minded the base ring.
comment:29 in reply to: ↑ 28 Changed 3 years ago by
Replying to ghkliem:
Replying to jipilab:
The doc should include in the description of backend input of both functions:
If ``None`` then ``backend='ppl'`` is used.Add the period at the end of the sentence.
A semicolon would probably fit better. I learnt that INPUT does not stop with period.
That's correct. In this case, it is a sentence with a noun, a verb and and complement. Thus it requires a period. Usually it is not a sentence.......................
Why is the only allowed base ring
As for the flow polytope, one can add an argument
base_ring
later.As for the Associahedron, one would need one class for each combination of
base_ring
andbackend
(for each child ofPolyhedron_base
). Is it worth it? I don't see a great need for a different base ring. (Contrary, constructing an associahedron with normaliz right away, saves a lot of time.)Anyway, the goal of this ticket was to add a backend option. So I never minded the base ring.
Ok, fine.
comment:30 followup: ↓ 31 Changed 3 years ago by
 Commit changed from f99ab5afb6f8f2de5a823d8dee45e5d809a99867 to d9a7e6b76dfad44bb6951099ae068c5dd0c0de6d
Branch pushed to git repo; I updated commit sha1. New commits:
d9a7e6b  periods to ends of sentences

comment:31 in reply to: ↑ 30 Changed 3 years ago by
comment:32 Changed 3 years ago by
 Status changed from needs_review to needs_work
red branch, meaning that it needs to be rebased on the latest beta
comment:33 Changed 3 years ago by
 Commit changed from d9a7e6b76dfad44bb6951099ae068c5dd0c0de6d to 6cffe8d25071280c913120c25ed45cb0509d493a
Branch pushed to git repo; I updated commit sha1. New commits:
6cffe8d  added `backend` to associahedron and flow polytope

comment:34 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:35 followup: ↓ 37 Changed 3 years ago by
It seems like you have some extra stuff in digraph
that has snuck in with the all_paths_iterator
.
comment:36 Changed 3 years ago by
 Commit changed from 6cffe8d25071280c913120c25ed45cb0509d493a to 949b0a943b35aa4474d35890bf2fa5d966c6641f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
949b0a9  added `backend` to associahedron and flow polytope

comment:37 in reply to: ↑ 35 Changed 3 years ago by
Replying to tscrim:
It seems like you have some extra stuff in
digraph
that has snuck in with theall_paths_iterator
.
Done.
comment:38 followup: ↓ 40 Changed 3 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to needs_work
Okay, I am pretty sure you now break Associahedra
when the backend is not specified. I also do not see why you need this in Associahedron
:
if backend is None: backend = 'ppl'
Just set backend='ppl'
as the default argument instead of None
.
You should also update the documentation there too:
 ``backend``  string (``'ppl'``); the backend to use; see :meth:`sage.geometry.polyhedron.constructor.Polyhedron`
Also, in flow_polytope
:
 ``backend``  string or ``None`` (default); the backend to use; see :meth:`sage.geometry.polyhedron.constructor.Polyhedron`
(By convention in Sage, the INPUT:
block points are not considered to be full sentences, so no period/fullstop.)
comment:39 Changed 3 years ago by
 Commit changed from 949b0a943b35aa4474d35890bf2fa5d966c6641f to e13719990d626cef945852e79d8ef68d92a99554
Branch pushed to git repo; I updated commit sha1. New commits:
e137199  changed default value of `backend`; small changes in documentation

comment:40 in reply to: ↑ 38 Changed 3 years ago by
Replying to tscrim:
You should also update the documentation there too:
 ``backend``  string (``'ppl'``); the backend to use; see :meth:`sage.geometry.polyhedron.constructor.Polyhedron`Also, in
flow_polytope
: ``backend``  string or ``None`` (default); the backend to use; see :meth:`sage.geometry.polyhedron.constructor.Polyhedron`(By convention in Sage, the
INPUT:
block points are not considered to be full sentences, so no period/fullstop.)
We had the discussion before. I guess its fine now, as we got rid of the complete sentences.
Btw, in many (if not all) polytopes in the library the INPUT:
block is a complete sentence with period/fullstop.
comment:41 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:42 Changed 3 years ago by
 Status changed from needs_review to positive_review
Thank you. Yea, well, there have been some fairly inconsistent things done with the documentation over the years. :/
I am at least trying to move towards what the standard in the dev guide
comment:43 Changed 3 years ago by
 Reviewers changed from Travis Scrimshaw to JeanPhilippe Labbé, Travis Scrimshaw
comment:44 Changed 3 years ago by
 Branch changed from public/27798 to e13719990d626cef945852e79d8ef68d92a99554
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
added `backend` to associahedron and flow polytope