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

Features for palp, polytopes_db, polytopes_db_4d #32893

Closed
mkoeppe opened this issue Nov 18, 2021 · 30 comments
Closed

Features for palp, polytopes_db, polytopes_db_4d #32893

mkoeppe opened this issue Nov 18, 2021 · 30 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 18, 2021

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Nov 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

Branch: u/mkoeppe/feature_for_palp

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

Commit: 6be27b7

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

New commits:

aee4ad2src/sage/features/palp.py: New
73b5b50src/sage/geometry/polyhedron/palp_database.py: Add # sage.doctest: optional - palp
55a228dsrc/sage/geometry/lattice_polytope.py: Mark doctests # optional - palp
6be27b7src/sage/geometry: Mark doctests # optional - palp

@mkoeppe

This comment has been minimized.

@novoselt
Copy link
Member

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

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?

@novoselt
Copy link
Member

comment:7

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

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

74fdacfLatticePolytopeClass, NefPartition, PointCollection, ToricLattice_ambient: Add `_sage_input_` methods

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

Changed commit from 6be27b7 to 74fdacf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

Changed commit from 74fdacf to 9d726ce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

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

9d726ceLatticePolytopeClass, NefPartition, PointCollection, ToricLattice_ambient: Add `_sage_input_` methods

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

Changed commit from 9d726ce to 1c78501

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 22, 2022

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

Changed commit from 1c78501 to 1025d65

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

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

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

@mkoeppe

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

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

55e812csrc/sage/geometry/lattice_polytope.py: Fix typo

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2022

Changed commit from 1025d65 to 55e812c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2022

Changed commit from 55e812c to d97479d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2022

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

d97479dsrc/sage/features/databases.py (DatabaseReflexivePolytopes): New

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Feature for palp Feature for palp, polytopes_db, polytopes_db_4d Feb 23, 2022
@mkoeppe mkoeppe changed the title Feature for palp, polytopes_db, polytopes_db_4d Features for palp, polytopes_db, polytopes_db_4d Feb 23, 2022
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 3, 2022

comment:19

Doctest failures are unrelated, needs review

@dimpase
Copy link
Member

dimpase commented Mar 5, 2022

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Mar 5, 2022

comment:20

lgtm

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 5, 2022

comment:21

Thank you!

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 5, 2022

comment:22

Follow-up: #33467

@vbraun
Copy link
Member

vbraun commented Mar 9, 2022

Changed branch from u/mkoeppe/feature_for_palp to d97479d

@vbraun vbraun closed this as completed in 950ecf8 Mar 9, 2022
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
mkoeppe added a commit to mkoeppe/sage that referenced this issue Feb 12, 2023
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

4 participants