Opened 7 years ago

Closed 3 years ago

#15253 closed defect (fixed)

Cartesian product of polyhedra with different dimension fails

Reported by: vbraun Owned by:
Priority: major Milestone: sage-8.0
Component: geometry Keywords:
Cc: dimpase, jipilab, mforets, vdelecroix Merged in:
Authors: Marcelo Forets Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: a95ca7f (Commits) Commit: a95ca7f209ee78d2e873210e939e16031dd47a62
Dependencies: Stopgaps:

Description

sage: polytopes.n_cube(1) * polytopes.n_cube(2)
...
TypeError: no common canonical parent for objects with parents: 'Polyhedra in ZZ^1' and 'Polyhedra in ZZ^2'

probably shouldn't use @coerce_binop on Polyhedra.product

Change History (15)

comment:1 Changed 7 years ago by dimpase

  • Cc dimpase added

comment:2 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 3 years ago by mforets

  • Cc jipilab mforets added

comment:6 Changed 3 years ago by mforets

i read coerce_binop at sage/structure/element.pyx, but do not understand what is the use case for Polyhedron, can someone give some hints?

when you remove the @coerce_binop decorator, the example in product (/polyhedron/base.py) still transforms to QQ^2 when asked for P1 * P2.

comment:7 Changed 3 years ago by mforets

would it be ok if it just performs a has_coerce_map_from for the base rings?

comment:8 Changed 3 years ago by mforets

Hi, this is not a solution! i'm posting a commented session where i learned something new about this issue..

first, the problem:

sage: P = polytopes.hypercube(1)
sage: Q = polytopes.hypercube(2)
sage: P * Q
...
TypeError: no common canonical parent for objects with parents: 'Polyhedra in ZZ^1' and 'Polyhedra in ZZ^2'

it arises from using the @coerce_binop decorator at the definition of (Cartesian) product. indeed,

sage: P.parent()
Polyhedra in ZZ^1
sage: Q.parent()
Polyhedra in ZZ^2
sage: P.parent().parent()
<class 'sage.geometry.polyhedron.parent.Polyhedra_ZZ_ppl_with_category'>
sage: Q.parent().parent()
<class 'sage.geometry.polyhedron.parent.Polyhedra_ZZ_ppl_with_category'>

in consequence:

sage: P.parent() is Q.parent()  # ok
False
sage: P.parent().parent() is Q.parent().parent()  # ok
True

on the other side, in def coerce_binop(method) it uses have_same_parent:

sage: from sage.structure.element import have_same_parent
sage: have_same_parent(P, Q)   # ok
False
sage: have_same_parent(P.parent(), Q.parent())   # why False?
False

so already at the level of coerce_binop there is this a bit strange thing one would say (?). also related is the Warning block in the definition of cpdef inline bint have_same_parent(left, right):, which says

This function assumes that at least one of the arguments is a Sage :class:Element. When in doubt, use the slower parent(left) is parent(right) instead.

To sum up:

  • perhaps one could define a custom @coerce_binop_polyhedron decorator which checks for grandparents, and which does not use have_same_parent.
  • alternatively, if it is indeed ok that if have_same_parent(self, other) gives False in this situation, then one should define a "canonical coercion" so that the method coercion_model.canonical_coercion(self, other) doesn't fail. but where?

comment:9 Changed 3 years ago by mforets

  • Cc vdelecroix added

CC'ing Vincent

comment:10 Changed 3 years ago by tscrim

Disclaimer: I am not an expert in polytopes. I feel like this is the correct error as there is not a canonical way to embed a Z polytope into a Z2 polytope.

However, that is not the issue with the ticket as the product is still well-defined, but what actually should be tested is that the base rings can be made into a common parent:

P.base_ring().has_coerce_map_from(Q.base_ring())

So if that is False, then an error should be raised. Otherwise it should change the base ring of both polytopes to this common ring and proceed.

comment:11 Changed 3 years ago by mforets

  • Branch set to u/mforets/15253
  • Commit set to a95ca7f209ee78d2e873210e939e16031dd47a62
  • Milestone changed from sage-6.4 to sage-8.0
  • Status changed from new to needs_review

Thanks Travis. I'm uploading an attempt based on the previous observations.

My disclaimer is that I need this operation to substitute 1 piece of my Matlab workflow :)


New commits:

a95ca7fchange decorator to typeerror exception

comment:12 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Positive review once you set the author field.

comment:13 Changed 3 years ago by dimpase

  • Authors set to Marcelo Forets
  • Status changed from needs_review to positive_review

comment:14 Changed 3 years ago by mforets

Thanks!

comment:15 Changed 3 years ago by vbraun

  • Branch changed from u/mforets/15253 to a95ca7f209ee78d2e873210e939e16031dd47a62
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.