Cartesian product of polyhedra with different dimension fails

vbraun commented 10 years ago

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

CC: @dimpase @jplab @mforets @videlec

Component: geometry

Author: Marcelo Forets

Branch/Commit: a95ca7f

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/15253

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:6

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.

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:7

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

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:8

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?

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:9

CC'ing Vincent

tscrim commented 7 years ago

comment:10

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 Z² 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.

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

Commit: a95ca7f

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:11

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:

a95ca7f change decorator to typeerror exception

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

Branch: u/mforets/15253

tscrim commented 7 years ago

comment:12

Positive review once you set the author field.

tscrim commented 7 years ago

Reviewer: Travis Scrimshaw

dimpase commented 7 years ago

Author: Marcelo Forets

c22b6800-ec0b-4cbf-94c4-0a74aecc2093 commented 7 years ago

comment:14

Thanks!

vbraun commented 7 years ago

Changed branch from u/mforets/15253 to a95ca7f

sagemath / sage

Cartesian product of polyhedra with different dimension fails #15253