Opened 2 years ago

Closed 20 months ago

#27798 closed enhancement (fixed)

Add `backend` option to associahedron and flow polytope

Reported by: gh-kliem Owned by:
Priority: major Milestone: sage-8.9
Component: geometry Keywords: polytopes, associahedron, flow polytopes, backend, normaliz
Cc: jipilab Merged in:
Authors: Jonathan Kliem Reviewers: Jean-Philippe Labbé, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: e137199 (Commits, GitHub, GitLab) Commit: e13719990d626cef945852e79d8ef68d92a99554
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-kliem)

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

Change History (44)

comment:1 Changed 2 years ago by gh-kliem

  • Branch set to public/27798
  • Commit set to 26430784821c585ea4ab52327ac76ca18b7351b1

New commits:

2643078added `backend` to associahedron and flow polytope

comment:2 Changed 2 years ago by git

  • Commit changed from 26430784821c585ea4ab52327ac76ca18b7351b1 to a9b4826fd8db7a30960d203cc89a4e937c8785fc

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

a9b4826typo

comment:3 follow-ups: Changed 2 years ago by slabbe

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

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

comment:4 Changed 2 years ago by git

  • Commit changed from a9b4826fd8db7a30960d203cc89a4e937c8785fc to 87169854f26eab8a4030efef1869a1883500b336

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

8716985corrected docstring

comment:5 Changed 2 years ago by gh-kliem

  • Authors set to Jonathan Kliem
  • 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 2 years ago by 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.

comment:7 Changed 2 years ago by gh-kliem

  • 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 2 years ago by git

  • Commit changed from 87169854f26eab8a4030efef1869a1883500b336 to b873156d9459e778e560d9a9d499002e29470011

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

b873156associahedron actually uses the claimed backend now

comment:9 Changed 2 years ago by gh-kliem

  • 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 follow-up: Changed 2 years ago by 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.

comment:11 in reply to: ↑ 10 Changed 2 years ago by 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.

comment:12 Changed 2 years ago by git

  • Commit changed from b873156d9459e778e560d9a9d499002e29470011 to 1f4bb257102026e66791f0c25547d7a8767467ac

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

1f4bb25comments by tscrim

comment:13 Changed 2 years ago by jipilab

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

comment:14 Changed 2 years ago by git

  • Commit changed from 1f4bb257102026e66791f0c25547d7a8767467ac to 2c812c508f0f5d7a8a88f132e32b149e4cc58788

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

2c812c5should have fixed bug in #25183

comment:15 Changed 2 years ago by chapoton

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 2 years ago by git

  • Commit changed from 2c812c508f0f5d7a8a88f132e32b149e4cc58788 to 65b11c9de95bb2ecde90c722ca2e9781fdac7cc5

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

65b11c9removed redundant line

comment:17 follow-up: Changed 2 years ago by 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?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by 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?

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by 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.

comment:20 in reply to: ↑ 19 ; follow-up: Changed 2 years ago by 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.

comment:21 in reply to: ↑ 20 Changed 2 years ago by 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.

comment:22 Changed 2 years ago by tscrim

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

comment:23 Changed 2 years ago by git

  • Commit changed from 65b11c9de95bb2ecde90c722ca2e9781fdac7cc5 to f99ab5afb6f8f2de5a823d8dee45e5d809a99867

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

f99ab5aremoved redundant import

comment:24 Changed 2 years ago by gh-kliem

Ok, I moved the corresponding part to #25183.

comment:25 Changed 22 months ago by gh-kliem

  • Milestone changed from sage-8.8 to sage-8.9

comment:26 Changed 22 months ago by gh-kliem

Is there anything left to take care of?

comment:27 in reply to: ↑ 3 ; follow-up: Changed 21 months ago by 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.

Why is the only allowed base ring QQ? Am I missing something?

comment:28 in reply to: ↑ 27 ; follow-up: Changed 21 months ago by 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.

comment:29 in reply to: ↑ 28 Changed 21 months ago by jipilab

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.

comment:30 follow-up: Changed 21 months ago by git

  • Commit changed from f99ab5afb6f8f2de5a823d8dee45e5d809a99867 to d9a7e6b76dfad44bb6951099ae068c5dd0c0de6d

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

d9a7e6bperiods to ends of sentences

comment:31 in reply to: ↑ 30 Changed 21 months ago by gh-kliem

Replying to git:

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

d9a7e6bperiods to ends of sentences

Took care of the periods.

comment:32 Changed 20 months ago by chapoton

  • Status changed from needs_review to needs_work

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

comment:33 Changed 20 months ago by git

  • Commit changed from d9a7e6b76dfad44bb6951099ae068c5dd0c0de6d to 6cffe8d25071280c913120c25ed45cb0509d493a

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

6cffe8dadded `backend` to associahedron and flow polytope

comment:34 Changed 20 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:35 follow-up: Changed 20 months ago by tscrim

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

comment:36 Changed 20 months ago by git

  • Commit changed from 6cffe8d25071280c913120c25ed45cb0509d493a to 949b0a943b35aa4474d35890bf2fa5d966c6641f

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

949b0a9added `backend` to associahedron and flow polytope

comment:37 in reply to: ↑ 35 Changed 20 months ago by 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.

comment:38 follow-up: Changed 20 months ago by tscrim

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

comment:39 Changed 20 months ago by git

  • Commit changed from 949b0a943b35aa4474d35890bf2fa5d966c6641f to e13719990d626cef945852e79d8ef68d92a99554

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

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

comment:40 in reply to: ↑ 38 Changed 20 months ago by 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.

comment:41 Changed 20 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:42 Changed 20 months ago by tscrim

  • 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 20 months ago by tscrim

  • Reviewers changed from Travis Scrimshaw to Jean-Philippe Labbé, Travis Scrimshaw

comment:44 Changed 20 months ago by vbraun

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