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

Set up permutahedron with both Vrep and Hrep (if backend supports it) #29325

Closed
kliem opened this issue Mar 13, 2020 · 28 comments
Closed

Set up permutahedron with both Vrep and Hrep (if backend supports it) #29325

kliem opened this issue Mar 13, 2020 · 28 comments

Comments

@kliem
Copy link
Contributor

kliem commented Mar 13, 2020

We set up the permutahedron with both Vrepresenation and Hrepresentation, if the backend supports it.

If the backend supports precomputed data (currently field) this is much faster. Otherwise, this is a bit faster, as the Hrepresentation seems to be the better choice.

Before this ticket:

sage: %timeit P = polytopes.permutahedron(6)  # using ppl
10 loops, best of 5: 56.5 ms per loop
sage: %timeit P = polytopes.permutahedron(7)  # using ppl
1 loop, best of 5: 1.57 s per loop

With this ticket:

sage: %timeit P = polytopes.permutahedron(6)  # using ppl
10 loops, best of 5: 34.2 ms per loop
sage: %timeit P = polytopes.permutahedron(7)  # using ppl
1 loop, best of 5: 1.04 s per loop
sage: %time P = polytopes.permutahedron(8, backend='field')
CPU times: user 587 ms, sys: 20 ms, total: 607 ms
Wall time: 607 ms
sage: %time P = polytopes.permutahedron(9, backend='field')
CPU times: user 5.08 s, sys: 248 ms, total: 5.33 s
Wall time: 5.33 s

Note that field is much slower than ppl before this ticket and the timings are therefore omitted.

As the order of Vrepresentation and Hrepresentation changes, a lot of tests need to be fixed.

CC: @jplab @LaisRast

Component: geometry

Keywords: polytopes, precomputed data, permutahedron

Author: Jonathan Kliem

Branch/Commit: 577e736

Reviewer: Jean-Philippe Labbé, Matthias Koeppe

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

@kliem kliem added this to the sage-9.1 milestone Mar 13, 2020
@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

New commits:

33e4490set up permutahedron with precomputed data
32708d4fixing failing doctests due to reordering

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Commit: 32708d4

@kliem
Copy link
Contributor Author

kliem commented Mar 13, 2020

Branch: public/29325

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 14, 2020

comment:2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 14, 2020
@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

Changed commit from 32708d4 to 3174e62

@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

New commits:

24f2a4aset up permutahedron with precomputed data
3174e62fixing failing doctests due to reordering

@kliem
Copy link
Contributor Author

kliem commented Apr 24, 2020

Changed branch from public/29325 to public/29325-reb2

@jplab
Copy link

jplab commented May 11, 2020

Reviewer: Jean-Philippe Labbé

@jplab
Copy link

jplab commented May 11, 2020

comment:4

These timings are interesting. Perhaps it would be good to add them to the documentation of the function? Since the backend field performs well but isn't the default, it would be good to give this information in the documentation.

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

Changed branch from public/29325-reb2 to public/29325-reb3

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

New commits:

ddb39afset up permutahedron with precomputed data
5d2e748fixing failing doctests due to reordering
dd259d7tuple -> generator
37f8c0ddocumentated that backend field is fast

@kliem
Copy link
Contributor Author

kliem commented May 11, 2020

Changed commit from 3174e62 to 37f8c0d

@jplab
Copy link

jplab commented May 12, 2020

comment:6

Failing doctests, see patchbot:

sage -t --long src/doc/en/thematic_tutorials/geometry/tips.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/geometry/tips.rst", line 110, in doc.en.thematic_tutorials.geometry.tips
Failed example:
    print(P.Hrepresentation_str(style='<='))
Expected:
    -x0 - x1 - x2 == -6
          x1 + x2 <=  5
               x2 <=  3
               x1 <=  3
              -x1 <= -1
         -x1 - x2 <= -3
              -x2 <= -1
Got:
    -x0 - x1 - x2 == -6 
         -x0 - x1 <= -3 
          x0 + x1 <=  5 
              -x1 <= -1 
               x0 <=  3 
              -x0 <= -1 
               x1 <=  3 
**********************************************************************
File "src/doc/en/thematic_tutorials/geometry/tips.rst", line 118, in doc.en.thematic_tutorials.geometry.tips
Failed example:
    print(P.Hrepresentation_str(style='positive'))
Expected:
    x0 + x1 + x2 == 6
               5 >= x1 + x2
               3 >= x2
               3 >= x1
              x1 >= 1
         x1 + x2 >= 3
              x2 >= 1
Got:
    x0 + x1 + x2 == 6      
         x0 + x1 >= 3      
               5 >= x0 + x1
              x1 >= 1      
               3 >= x0     
              x0 >= 1      
               3 >= x1     
**********************************************************************
----------------------------------------------------------------------
sage -t --long src/sage/schemes/cyclic_covers/charpoly_frobenius.py  # 1 doctest failed
sage -t --long src/sage/geometry/polyhedron/library.py  # Bad exit: 1
sage -t --long src/doc/en/thematic_tutorials/geometry/tips.rst  # 2 doctests failed
----------------------------------------------------------------------

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

Changed commit from 37f8c0d to 29bb85b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 12, 2020

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

29bb85bfix doctests

@kliem
Copy link
Contributor Author

kliem commented May 12, 2020

comment:8

As for the bad exit it appears that memory consumption is highly unstable during doctesting.
I can make it pass with 2400 MB and default is 3300 MB, so I would expect this to work anywhere, but I guess this is not the case. (And 2400 MB is much over what causes a bad exit for Sébastien Labbé).

I guess I just mark it as not tested. The examples are more for documentation than for testing.


New commits:

29bb85bfix doctests

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

comment:10

"not tested" seems like the right choice for this.

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 11, 2020

comment:11

Needs rebase

@kliem
Copy link
Contributor Author

kliem commented Aug 11, 2020

Changed commit from 29bb85b to 577e736

@kliem
Copy link
Contributor Author

kliem commented Aug 11, 2020

New commits:

56d95ebset up permutahedron with precomputed data
2325e66fixing failing doctests due to reordering
aa1afe8tuple -> generator
b28e980documentated that backend field is fast
1bfcf5dfix doctests
577e736use test suite to check precomputed data

@kliem
Copy link
Contributor Author

kliem commented Aug 11, 2020

Changed branch from public/29325-reb3 to public/29325-reb4

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 12, 2020

comment:14

these doctests that seem to depend on an unspecified order of enumeration... is there not a better way of testing this, for example by sorting...?

@kliem
Copy link
Contributor Author

kliem commented Aug 12, 2020

comment:15

There is only a few doctests that can be fixed by sorting. The face iterator really behaves different with different order and the indices are all shuffled.

@kliem
Copy link
Contributor Author

kliem commented Aug 12, 2020

comment:16

And the first ones cannot be fixed by this either. Looks like ppl picks a different representative for the inequalities (that are modulo an equation).

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 12, 2020

Changed reviewer from Jean-Philippe Labbé to Jean-Philippe Labbé, Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 12, 2020

comment:18

Ok then

@kliem
Copy link
Contributor Author

kliem commented Aug 13, 2020

comment:19

Thank you.

@vbraun
Copy link
Member

vbraun commented Aug 15, 2020

Changed branch from public/29325-reb4 to 577e736

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