Features for palp, polytopes_db, polytopes_db_4d

Question

Features for palp, polytopes_db, polytopes_db_4d

Closed this issue 4 years ago · 30 comments

palp is a standard package, but with modularization that does not mean that everyone has it. In particular, sagemath-polyhedra (#32432) will not ship it.

We add a feature test along the lines of the palp spkg-configure.m4 script and mark many doctests in sage.geometry as # optional - palp.

To help construct doctest examples without having to rely on palp, we endow several classes with _sage_input_ methods.

As a cosmetic change, nef-partitions now use a unicode symbol.

-            Nef-partition {0, 1, 3} U {2, 4, 5}
+            Nef-partition {0, 1, 3} ⊔ {2, 4, 5}

We also add feature tests for polytopes_db, polytopes_db_4d.

CC: @kliem @novoselt @seblabbe @dimpase

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: d97479d

Reviewer: Dima Pasechnik

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

Answer 1 · 2022-02-07T03:09:20.000Z

Description changed:

--- 
+++ 
@@ -1,2 +1,3 @@
 `palp` is a standard package, but with modularization that does not mean that everyone has it.
 
+We should add a feature test along the lines of the palp `spkg-configure.m4` script

Answer 2 · 2022-02-22T07:54:49.000Z

Branch: u/mkoeppe/feature_for_palp

Answer 3 · 2022-02-22T07:56:51.000Z

Author: Matthias Koeppe

Answer 4 · 2022-02-22T07:56:51.000Z

Commit: 6be27b7

Answer 5 · 2022-02-22T07:56:51.000Z

New commits:

`aee4ad2`	`src/sage/features/palp.py: New`
`73b5b50`	`src/sage/geometry/polyhedron/palp_database.py: Add # sage.doctest: optional - palp`
`55a228d`	`src/sage/geometry/lattice_polytope.py: Mark doctests # optional - palp`
`6be27b7`	`src/sage/geometry: Mark doctests # optional - palp`

Answer 6 · 2022-02-22T07:56:51.000Z

Description changed:

--- 
+++ 
@@ -1,3 +1,4 @@
-`palp` is a standard package, but with modularization that does not mean that everyone has it.
+`palp` is a standard package, but with modularization that does not mean that everyone has it. In particular, **sagemath-polyhedra** (#32432) will not ship it.
 
-We should add a feature test along the lines of the palp `spkg-configure.m4` script
+We add a feature test along the lines of the palp `spkg-configure.m4` script and mark many doctests in `sage.geometry` as `# optional - palp`.
+

Answer 7 · 2022-02-22T17:18:10.000Z

comment:5

I find such markings ridiculous... If a module is useless without PALP, it would make much more sense to me to mark the module once as optional and not on every single line of examples.

My random guess is that modules for toric varieties will mostly stop working as well and again it would make much more sense to mark those modules as dependent on PALP and not each line.

Answer 8 · 2022-02-22T17:51:43.000Z

comment:6

Thanks for the feedback. I wouldn't go so far as to call the module useless. The explicit interaction of the polytope with a lattice in this class is valuable design and code that is not tied to the choice of PALP as a backend.

There is a lot of duplication / proliferation of incompatible variants among sage.geometry.lattice_polytope, sage.geometry.polyhedron.ppl_lattice_polytope, sage.geometry.polyhedron that is calling for refactoring. To mark which lines of lattice_polytope actually depend on palp (about 1/3 of the sage: lines in this file) was a first step to inform possible such refactoring steps.

Many of the # optional markings in tests for NefPartition could be avoided by constructing the example explicitly instead of using

        sage: o = lattice_polytope.cross_polytope(3)
        sage: np = o.nef_partitions()[0]; np                                    # optional - palp

repeatedly. Would this be agreeable?

Answer 9 · 2022-02-22T18:08:14.000Z

comment:7

Since I am not doing any work anymore, it is up to the people who do to agree on ;-)

Answer 10 · 2022-02-22T20:02:49.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`74fdacf`	LatticePolytopeClass, NefPartition, PointCollection, ToricLattice_ambient: Add `_sage_input_` methods

Answer 11 · 2022-02-22T20:02:49.000Z

Changed commit from 6be27b7 to 74fdacf

Answer 12 · 2022-02-22T20:11:42.000Z

Changed commit from 74fdacf to 9d726ce

Answer 13 · 2022-02-22T20:11:42.000Z

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

`9d726ce`	LatticePolytopeClass, NefPartition, PointCollection, ToricLattice_ambient: Add `_sage_input_` methods

Answer 14 · 2022-02-22T20:25:23.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`1c78501`	`src/sage/geometry/lattice_polytope.py: In NefPartition doctests, construct examples without using palp`

Answer 15 · 2022-02-22T20:25:23.000Z

Changed commit from 9d726ce to 1c78501

Answer 16 · 2022-02-22T21:20:01.000Z

comment:11

Replying to @mkoeppe:

To mark which lines of lattice_polytope actually depend on palp (about 1/3 of the sage: lines in this file)

down to less than 1/4 of the lines now

Answer 17 · 2022-02-22T21:27:43.000Z

Changed commit from 1c78501 to 1025d65

Answer 18 · 2022-02-22T21:27:43.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`1025d65`	`src/sage/geometry/lattice_polytope.py, src/sage/schemes/toric/fano_variety.py: Use unicode disjoint union symbol`

Answer 19 · 2022-02-22T21:32:35.000Z

Description changed:

--- 
+++ 
@@ -2,3 +2,12 @@
 
 We add a feature test along the lines of the palp `spkg-configure.m4` script and mark many doctests in `sage.geometry` as `# optional - palp`.
 
+To help construct doctest examples without having to rely on `palp`, we endow several classes with `_sage_input_` methods.
+
+As a cosmetic change, nef-partitions now use a unicode symbol.
+
+```
+-            Nef-partition {0, 1, 3} U {2, 4, 5}
++            Nef-partition {0, 1, 3} ⊔ {2, 4, 5}
+```
+

Answer 20 · 2022-02-22T22:38:59.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`55e812c`	`src/sage/geometry/lattice_polytope.py: Fix typo`

Answer 21 · 2022-02-22T22:38:59.000Z

Changed commit from 1025d65 to 55e812c

Answer 22 · 2022-02-23T00:37:17.000Z

Changed commit from 55e812c to d97479d

Answer 23 · 2022-02-23T00:37:17.000Z

Branch pushed to git repo; I updated commit sha1. New commits:

`d97479d`	`src/sage/features/databases.py (DatabaseReflexivePolytopes): New`

Answer 24 · 2022-02-23T00:38:39.000Z

Description changed:

--- 
+++ 
@@ -11,3 +11,5 @@
 +            Nef-partition {0, 1, 3} ⊔ {2, 4, 5}
 ```
 
+We also add feature tests for `polytopes_db`, `polytopes_db_4d`.
+

Answer 25 · 2022-03-03T01:53:24.000Z

comment:19

Doctest failures are unrelated, needs review

Answer 26 · 2022-03-05T10:13:03.000Z

Reviewer: Dima Pasechnik

Answer 27 · 2022-03-05T10:13:03.000Z

comment:20

lgtm

Answer 28 · 2022-03-05T18:04:09.000Z

comment:21

Thank you!

Answer 29 · 2022-03-05T20:57:49.000Z

comment:22

Follow-up: #33467

Answer 30 · 2022-03-09T23:38:17.000Z

Changed branch from u/mkoeppe/feature_for_palp to d97479d