Opened 5 years ago

Last modified 3 years ago

#25183 closed defect

Bug in the associahedron object — at Version 9

Reported by: Jean-Philippe Labbé Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: associahedron
Cc: Frédéric Chapoton, Christian Stump, Travis Scrimshaw, Moritz Firsching, Jean-Philippe Labbé, Laith Rastanawi Merged in:
Authors: Jonathan Kliem Reviewers:
Report Upstream: N/A Work issues:
Branch: public/25183 (Commits, GitHub, GitLab) Commit: 0e892e7019001400faf0c3cfed542fffd3ac7227
Dependencies: #27798 Stopgaps:

Status badges

Description (last modified by gh-kliem)

The following lines currently occur:

sage: A = polytopes.associahedron(['A',3])
sage: face = A.faces(2)[3]
sage: face
sage: face.as_polyhedron()
/home/jplabbe/sage/local/lib/python2.7/site-packages/sage/repl/rich_output/ RichReprWarning: Exception in _rich_repr_ while displaying object: 'Associahedra_with_category.element_class' object has no attribute '_cartan_type'
<repr(<sage.combinat.root_system.associahedron.Associahedra_with_category.element_class at 0x7f48d0d7ad70>) failed: AttributeError: 'Associahedra_with_category.element_class' object has no attribute '_cartan_type'>

The construction as_polyhedron initializes the new (face-)polyhedron via P.__class__(parent, Vrep, None). In the case of the associahedron, this does not give a valid object, as the associahedron requires a cartan type as well.

Same (or similar problem) with:

  • minkowski_sum,
  • minkowski_difference,
  • translation,
  • dilation,
  • convex_hull,
  • intersection (actually even worse as coercion fails),
  • polar.

We fix the initialization of the associahedron to now require the cartan type on __init__ (before it was assumed to be set after initialization).

Further we have __new__ return the correct parent class such that e.g. the face of an Associahedron_class_ppl is constructed as Polyhedron_ppl.

We fix _coerce_map_from_ to take into account that no general polyhedron can be coerced to an associahedron.

We manually set the correct pushout of polyhedra over ZZ and associahedra (over QQ) to be polyhedra over QQ.

Change History (9)

comment:1 Changed 5 years ago by Moritz Firsching

Cc: Moritz Firsching added

comment:2 Changed 4 years ago by gh-kliem

I think I took care of it in #27798.

comment:3 Changed 4 years ago by gh-kliem

Authors: Jonathan Kliem
Branch: public/25183
Commit: 65b11c9de95bb2ecde90c722ca2e9781fdac7cc5
Dependencies: 27798
Description: modified (diff)
Status: newneeds_review

This branch should fix the bug.

Feel free to change it, alter it, abonden it, etc.

Maybe there is a better way to obtain the correct parent, i.e. improve the following lines:

156             for typ1 in ancestors_of_associahedron:
157                 if typ1 in mro:
158                     return typ1(parent, Vrep, Hrep, **kwds)

New commits:

2643078added `backend` to associahedron and flow polytope
8716985corrected docstring
b873156associahedron actually uses the claimed backend now
1f4bb25comments by tscrim
2c812c5should have fixed bug in #25183
65b11c9removed redundant line

comment:4 Changed 4 years ago by gh-kliem

Dependencies: 27798#27798

comment:5 Changed 3 years ago by gh-kliem

Milestone: sage-8.2sage-8.9

comment:6 Changed 3 years ago by gh-kliem

Status: needs_reviewneeds_work

Once #27798 is done, I'm going to rebase it and solve the merge conflict. At the moment it seems pointless to me.

comment:7 Changed 3 years ago by gh-kliem

Description: modified (diff)

comment:8 Changed 3 years ago by git

Commit: 65b11c9de95bb2ecde90c722ca2e9781fdac7cc50e892e7019001400faf0c3cfed542fffd3ac7227

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

949b0a9added `backend` to associahedron and flow polytope
e137199changed default value of `backend`; small changes in documentation
0e892e7modify construction and coercion of associahedra to fix bug #25183

comment:9 Changed 3 years ago by gh-kliem

Description: modified (diff)
Status: needs_workneeds_review
Note: See TracTickets for help on using tickets.