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

Cythonize LatticePolytope.normal_form(algorithm='palp_native'), change to default, add as a Polyhedron method #36031

Merged
merged 36 commits into from
Dec 6, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Aug 3, 2023

The current default algorithm, using the external, unmaintained palp package, is unreliable.

Here, as a follow-up on #35997, we make the reimplementation of the PALP algorithm in Python (palp_native) suitable as the new default by speeding it up.

We also fix a bug in introduced in #35997.

We also make it available as a method of Polyhedron (for base ring ZZ).

@xuluze

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe self-assigned this Aug 3, 2023
@mkoeppe mkoeppe changed the title Cythonize PALP normal form Cythonize PALP normal form, make it available as a Polyhedron method Aug 5, 2023
@mkoeppe mkoeppe marked this pull request as ready for review September 7, 2023 19:04
@mkoeppe mkoeppe changed the title Cythonize PALP normal form, make it available as a Polyhedron method Cythonize LatticePolytope.normal_form(algorithm='palp_native'), change to default, add as a Polyhedron method Sep 7, 2023
Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from that missing trivial test, no problems

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 22, 2023

Thanks

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 22, 2023

Marking this as a blocker in the hope it can sneak into 10.2 with the other open blocker

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 27, 2023
…alp_native')`, change to default, add as a `Polyhedron` method

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
The current default algorithm, using the external, unmaintained `palp`
package, is unreliable.

Here, as a follow-up on sagemath#35997, we make the reimplementation of the PALP
algorithm in Python (`palp_native`) suitable as the new default by
speeding it up.

We also fix a bug in introduced in sagemath#35997.

We also make it available as a method of `Polyhedron` (for base ring
`ZZ`).

@xuluze
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36031
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Frédéric Chapoton, Matthias Köppe
@tornaria
Copy link
Contributor

Marking this as a blocker in the hope it can sneak into 10.2 with the other open blocker

Why is this more worthy of "sneaking in" than e.g. #36551 ?

@vbraun
Copy link
Member

vbraun commented Nov 27, 2023

Fails on 32-bit

**********************************************************************
File "src/sage/geometry/lattice_polytope.py", line 3141, in sage.geometry.lattice_polytope.LatticePolytopeClass.normal_form
Failed example:
    P.normal_form(algorithm="palp_modified")  # long time (22s)           # needs sage.groups
Exception raised:
    Traceback (most recent call last):
      File "sage/misc/cachefunc.pyx", line 1962, in sage.misc.cachefunc.CachedMethodCaller.__call__
        return cache[k]
    KeyError: (('palp_modified', False), ())
    During handling of the above exception, another exception occurred:
    Traceback (most recent call last):
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.geometry.lattice_polytope.LatticePolytopeClass.normal_form[14]>", line 1, in <module>
        P.normal_form(algorithm="palp_modified")  # long time (22s)           # needs sage.groups
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/misc/cachefunc.pyx", line 1967, in sage.misc.cachefunc.CachedMethodCaller.__call__
        w = self._instance_call(*args, **kwds)
      File "sage/misc/cachefunc.pyx", line 1842, in sage.misc.cachefunc.CachedMethodCaller._instance_call
        return self.f(self._instance, *args, **kwds)
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/geometry/lattice_polytope.py", line 3204, in normal_form
        result = self._palp_modified_normal_form(permutation=permutation)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/geometry/lattice_polytope.py", line 3259, in _palp_modified_normal_form
        PM_max = PM.permutation_normal_form()
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/matrix/matrix2.pyx", line 8637, in genexpr
        if not any(aN.is_permutation_of(j) for j in aM):
      File "sage/matrix/matrix2.pyx", line 8637, in genexpr
        if not any(aN.is_permutation_of(j) for j in aM):
      File "sage/matrix/matrix2.pyx", line 8792, in sage.matrix.matrix2.Matrix.is_permutation_of
        return N_B.is_isomorphic(M_B, certificate=False, edge_labels=True)
      File "/var/lib/buildbot/slave/sage_git/build/src/sage/graphs/generic_graph.py", line 23852, in is_isomorphic
        isom = isomorphic(GC, GC2, partition, partition2, (self._directed or self.has_loops()), 1)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "sage/groups/perm_gps/partn_ref/refinement_graphs.pyx", line 166, in sage.groups.perm_gps.partn_ref.refinement_graphs.isomorphic
        cdef bint isomorphic = double_coset(<void *>GS1, <void *>GS2, part, ordering, n, &all_children_are_equivalent, &refine_by_degree, &compare_graphs, NULL, NULL, output)
      File "sage/groups/perm_gps/partn_ref/double_coset.pyx", line 367, in sage.groups.perm_gps.partn_ref.double_coset.double_coset
        raise MemoryError
    MemoryError
**********************************************************************
1 item had failures:
   1 of  17 in sage.geometry.lattice_polytope.LatticePolytopeClass.normal_form
    [667 tests, 1 failure, 16.73 s]
**********************************************************************

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 27, 2023

Sorry about this; I've disabled this example now (it's also too long on 64 bit even for # long)

Copy link

Documentation preview for this PR (built with commit f9aeca5; changes) is ready! 🎉

@mkoeppe mkoeppe removed this from the sage-10.2 milestone Dec 3, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
…alp_native')`, change to default, add as a `Polyhedron` method

    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
The current default algorithm, using the external, unmaintained `palp`
package, is unreliable.

Here, as a follow-up on sagemath#35997, we make the reimplementation of the PALP
algorithm in Python (`palp_native`) suitable as the new default by
speeding it up.

We also fix a bug in introduced in sagemath#35997.

We also make it available as a method of `Polyhedron` (for base ring
`ZZ`).

@xuluze
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36031
Reported by: Matthias Köppe
Reviewer(s): Dima Pasechnik, Frédéric Chapoton, Matthias Köppe
@vbraun vbraun merged commit c31f22e into sagemath:develop Dec 6, 2023
15 of 18 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 6, 2023
@mkoeppe mkoeppe deleted the palp_normal_form_cythonize branch February 24, 2024 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants