sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 462 forks source link

Coercion between algebraic polyhedra fails #28776

Open kliem opened 4 years ago

kliem commented 4 years ago

For the following polyhedra all things involving coercion fail.

sage: P = polytopes.icosahedron(); P
A 3-dimensional polyhedron in (Number Field in sqrt5 with defining polynomial x^2 - 5 with sqrt5 = 2.236067977499790?)^3 defined as the convex hull of 12 vertices
sage: P1 = polytopes.small_rhombicuboctahedron(); P1
A 3-dimensional polyhedron in (Number Field in sqrt2 with defining polynomial x^2 - 2 with sqrt2 = 1.414213562373095?)^3 defined as the convex hull of 24 vertices

It seems to boil down to two issues:

Do we want to change that design for polyhedra? I think it is reasonable to coerce polyhedra over number fields by default. At least when there is exactly one composite field.

Depends on #28770

CC: @jplab @LaisRast @videlec

Component: geometry

Keywords: algebraic polyhedra, quadratic fields

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

kliem commented 4 years ago

Description changed:

--- 
+++ 
@@ -12,4 +12,6 @@
 - `P.base_extend(P1.base_ring())` doesn't do anything, see #28770,
 - quadratic fields cannot be coerced at the moment, see #28774.

+Also, it would be great to coerce into the smallest number field possible. In the thematic tutorial for polytopes, it is pointed out that such a method already exists. #28778 will use this method for creating pushout of number fields, if possible. Then coercion of such polyhedra, will use the smallest number field possible by default.
+
 Once, both are taken care of, we should check that things work and add doctests accordingly.
kliem commented 4 years ago

Changed dependencies from #28770, #28774 to #28770, #28774, #28778

kliem commented 4 years ago

Changed dependencies from #28770, #28774, #28778 to #28770, #28774

kliem commented 4 years ago

Description changed:

--- 
+++ 
@@ -12,6 +12,6 @@
 - `P.base_extend(P1.base_ring())` doesn't do anything, see #28770,
 - quadratic fields cannot be coerced at the moment, see #28774.

-Also, it would be great to coerce into the smallest number field possible. In the thematic tutorial for polytopes, it is pointed out that such a method already exists. #28778 will use this method for creating pushout of number fields, if possible. Then coercion of such polyhedra, will use the smallest number field possible by default.
+Also, it would be great to coerce into the smallest number field possible. In the thematic tutorial for polytopes, it is pointed out that such a method already exists.

 Once, both are taken care of, we should check that things work and add doctests accordingly.
kliem commented 4 years ago
comment:3

As was pointed out to me, it is not desirable to coerce number fields to the smallest field in general. As it is not canonical, fragile and expensive.

However, it seems to be desirable for polyhedra, isn't it? Would it make sense to implement that somewhat as a standard behavior, e.g. that a polyhedron over QQ[sqrt(2)] and one over QQ[sqrt(3)] are coerced to one over QQ[sqrt(2) + sqrt(3)]?

kliem commented 4 years ago

Changed dependencies from #28770, #28774 to #28770

kliem commented 4 years ago

Description changed:

--- 
+++ 
@@ -10,8 +10,6 @@

 It seems to boil down to two issues:
 - `P.base_extend(P1.base_ring())` doesn't do anything, see #28770,
-- quadratic fields cannot be coerced at the moment, see #28774.
+- number fields can mostly not coerced by design.

-Also, it would be great to coerce into the smallest number field possible. In the thematic tutorial for polytopes, it is pointed out that such a method already exists.
-
-Once, both are taken care of, we should check that things work and add doctests accordingly.
+Do we want to change that design for polyhedra? I think it is reasonable to coerce polyhedra over number fields by default. At least when there is exactly one composite field.
embray commented 4 years ago
comment:5

Ticket retargeted after milestone closed

mkoeppe commented 4 years ago
comment:6

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

mkoeppe commented 3 years ago
comment:8

Moving this ticket to 9.4, as it seems unlikely that it will be merged in 9.3, which is in the release candidate stage

mkoeppe commented 3 years ago
comment:9

Setting a new milestone for this ticket based on a cursory review.

mkoeppe commented 2 years ago
comment:10

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.