Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cartesian product of polyhedra with different dimension fails #15253

Closed
vbraun opened this issue Oct 4, 2013 · 13 comments
Closed

Cartesian product of polyhedra with different dimension fails #15253

vbraun opened this issue Oct 4, 2013 · 13 comments

Comments

@vbraun
Copy link
Member

vbraun commented Oct 4, 2013

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

@vbraun vbraun added this to the sage-6.1 milestone Oct 4, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@mforets
Copy link
Mannequin

mforets mannequin commented Mar 10, 2017

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.

@mforets
Copy link
Mannequin

mforets mannequin commented Apr 11, 2017

comment:7

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

@mforets
Copy link
Mannequin

mforets mannequin commented May 4, 2017

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?

@mforets
Copy link
Mannequin

mforets mannequin commented May 4, 2017

comment:9

CC'ing Vincent

@tscrim
Copy link
Collaborator

tscrim commented May 10, 2017

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

@mforets
Copy link
Mannequin

mforets mannequin commented May 14, 2017

Commit: a95ca7f

@mforets
Copy link
Mannequin

mforets mannequin commented May 14, 2017

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:

a95ca7fchange decorator to typeerror exception

@mforets
Copy link
Mannequin

mforets mannequin commented May 14, 2017

Branch: u/mforets/15253

@mforets mforets mannequin modified the milestones: sage-6.4, sage-8.0 May 14, 2017
@mforets mforets mannequin added the s: needs review label May 14, 2017
@tscrim
Copy link
Collaborator

tscrim commented May 15, 2017

comment:12

Positive review once you set the author field.

@tscrim
Copy link
Collaborator

tscrim commented May 15, 2017

Reviewer: Travis Scrimshaw

@dimpase
Copy link
Member

dimpase commented May 15, 2017

Author: Marcelo Forets

@mforets
Copy link
Mannequin

mforets mannequin commented May 16, 2017

comment:14

Thanks!

@vbraun
Copy link
Member Author

vbraun commented May 16, 2017

Changed branch from u/mforets/15253 to a95ca7f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants