Add `backend` option to associahedron and flow polytope

[gh-kliem]

The option backend was missing for polytopes.associahedron and polytopes.flow_polytope.

[gh-kliem]

New commits:

​2643078

added `backend` to associahedron and flow polytope

[git]

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

​a9b4826

typo

[Sébastien Labbé]

The doc should include in the description of backend input of both functions:

If ``None`` then ``backend='ppl'`` is used.

[git]

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

​8716985

corrected docstring

[gh-kliem]

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.

[gh-kliem]

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.

[git]

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

​b873156

associahedron actually uses the claimed backend now

[gh-kliem]

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.

[Travis Scrimshaw]

Three little comments from a quick look:

Your INPUT: block is over-indented.

It is better to have new-style 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 class-level 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.

[gh-kliem]

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

It is better to have new-style 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 class-level 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.

[git]

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

​1f4bb25

comments by tscrim

[Jean-Philippe Labbé]

You may want to have a look at #25183 while you are playing around with the associahedron.

[git]

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

​2c812c5

should have fixed bug in #25183

[Frédéric Chapoton]

please fix the pyflakes warning:

+src/sage/combinat/root_system/associahedron.py:28:
 'sage.geometry.polyhedron.base.Polyhedron_base' imported but unused

[git]

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

​65b11c9

removed redundant line

[Travis Scrimshaw]

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?

[gh-kliem]

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 the Parent.element_class). Perhaps I am not quite understanding things. Could you explain a bit how you arrived at this?

[Jean-Philippe Labbé]

Replying to gh-kliem:

JP asked me to also fix #25183. So I did.

Have a look at it... If the bug is related or not.

[Jean-Philippe Labbé]

Replying to jipilab:

Replying to gh-kliem:

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.

[gh-kliem]

I misunderstood. From my understanding #25183 should be fixed with redefining __new__ or with changing the construction of as_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 gh-kliem:

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.

[Travis Scrimshaw]

I think it is better to have that discussion on #25183 as it is a separate issue.

[git]

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

​f99ab5a

removed redundant import

[gh-kliem]

Ok, I moved the corresponding part to #25183.

[gh-kliem]

Is there anything left to take care of?

[Jean-Philippe Labbé]

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?

[gh-kliem]

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 QQ? Am I missing something?

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.

[Jean-Philippe Labbé]

Replying to gh-kliem:

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 QQ? Am I missing something?

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.

Ok, fine.

[git]

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

​d9a7e6b

periods to ends of sentences

[gh-kliem]

Replying to git:

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

​d9a7e6b

periods to ends of sentences

Took care of the periods.

[Frédéric Chapoton]

red branch, meaning that it needs to be rebased on the latest beta

[git]

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

​6cffe8d

added `backend` to associahedron and flow polytope

[Travis Scrimshaw]

It seems like you have some extra stuff in digraph that has snuck in with the all_paths_iterator.

[git]

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

​949b0a9

added `backend` to associahedron and flow polytope

[gh-kliem]

Replying to tscrim:

It seems like you have some extra stuff in digraph that has snuck in with the all_paths_iterator.

Done.

[Travis Scrimshaw]

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/full-stop.)

[git]

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

​e137199

changed default value of `backend`; small changes in documentation

[gh-kliem]

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/full-stop.)

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/full-stop.

[Travis Scrimshaw]

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