Change minlex to use the linear time least_rotation.

[MarkCBell]

Currently the minlex utility function computes the earliest rotation of an iterable by producing all rotations of the iterable and looking for the smallest one. This is a quadratic time operation since there are linearly many rotations and it takes linear time to determine whether a rotation is smaller than the best rotation found so far. The utility function least_rotation uses Booth's algorithm to compute the index of the smallest rotation in linear time hence minlex can be fundamentally done using:
return left_rotate(seq, least_rotation(seq))
however a few extra cases are needed since minlex can also check for the smallest rotation of the iterable and its reverse.

Since this is now a linear time algorithm there is no benefit in passing in some of the hints that speed up the old function such as is_set and small.

• utilities
  • minlex no longer accepts is_set or small arguments
  • minlex and least_rotation now accept key= arguments similar to sorted.

[sympy-bot]

✅

Hi, I am the SymPy bot (v161). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• utilities

  • minlex no longer accepts is_set or small arguments (#20433 by @MarkCBell)

  • minlex and least_rotation now accept key= arguments similar to sorted. (#20433 by @MarkCBell)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.8.

Click here to see the pull request description that was parsed.

Currently the `minlex` utility function computes the earliest rotation of an iterable by producing all rotations of the iterable and looking for the smallest one. This is a quadratic time operation since there are linearly many rotations and it takes linear time to determine whether a rotation is smaller than the best rotation found so far. The utility function `least_rotation` uses Booth's algorithm to compute the index of the smallest rotation in linear time hence `minlex` can be fundamentally done using:
``` return left_rotate(seq, least_rotation(seq))```
however a few extra cases are needed since `minlex` can also check for the smallest rotation of the iterable and its reverse.

Since this is now a linear time algorithm there is no benefit in passing in some of the hints that speed up the old function such as `is_set` and `small`.

<!-- BEGIN RELEASE NOTES -->
* utilities
    * minlex no longer accepts is_set or small arguments
    * minlex and least_rotation now accept `key=` arguments similar to `sorted`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[codecov]

Codecov Report

Merging #20433 (9ad0ac5) into master (bd327ec) will decrease coverage by 0.025%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #20433       +/-   ##
=============================================
- Coverage   75.764%   75.739%   -0.026%     
=============================================
  Files          673       673               
  Lines       174119    174437      +318     
  Branches     41109     41200       +91     
=============================================
+ Hits        131921    132117      +196     
- Misses       36480     36602      +122     
  Partials      5718      5718               

[smichr]

This appears to no longer retain the string/tuple invariant for output.

[MarkCBell]

This appears to no longer retain the string/tuple invariant for output.

Thank you for letting me know about this. I have added a commit to ensure that the returned type is a tuple, unless the input is a string and have updated the docstring of the function to note that this is the desired behaviour.

[smichr]

It would be good to add a test for a string, too.

[smichr]

Using "default_sort_key" might be more restrictive than this was before. Things in iterables should mostly be sympy-agnostic. So previously the items were simply compared: if seq < best

[oscarbenjamin]

What is the reason for using default_sort_key?

[MarkCBell]

So without the default sort key the Polyhedron test (py.test -k test_args) fails since it tries to sort a bunch of Relationals which are not naturally orderable. The old version of this function got around this by using the default_sort_key method.

________________ test_sympy__combinatorics__polyhedron__Polyhedron _________________

    def test_sympy__combinatorics__polyhedron__Polyhedron():
        from sympy.combinatorics.permutations import Permutation
        from sympy.combinatorics.polyhedron import Polyhedron
        from sympy.abc import w, x, y, z
        pgroup = [Permutation([[0, 1, 2], [3]]),
                  Permutation([[0, 1, 3], [2]]),
                  Permutation([[0, 2, 3], [1]]),
                  Permutation([[1, 2, 3], [0]]),
                  Permutation([[0, 1], [2, 3]]),
                  Permutation([[0, 2], [1, 3]]),
                  Permutation([[0, 3], [1, 2]]),
                  Permutation([[0, 1, 2, 3]])]
        corners = [w, x, y, z]
        faces = [(w, x, y), (w, y, z), (w, z, x), (x, y, z)]
>       assert _test_args(Polyhedron(corners, faces, pgroup))

sympy/core/tests/test_args.py:514: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
sympy/combinatorics/polyhedron.py:389: in __new__
    faces = [minlex(f, directed=False) for f in faces]
sympy/combinatorics/polyhedron.py:389: in <listcomp>
    faces = [minlex(f, directed=False) for f in faces]
sympy/utilities/iterables.py:2430: in minlex
    best = rotate_left(seq, least_rotation(seq))
sympy/utilities/iterables.py:1309: in least_rotation
    if (sj) < (S[k]):

self = x < w

    def __bool__(self):
>       raise TypeError("cannot determine truth value of Relational")
E       TypeError: cannot determine truth value of Relational

sympy/core/relational.py:395: TypeError

[oscarbenjamin]

Sorry, I didn't see that default_sort_key was also being used in the old implementation.

@smichr does this look good to you?

(I don't know this particular routine so well but the change seems reasonable to me from a high level)

[MarkCBell]

So having given this some thought, I think one refinement that would be reasonable is to adjust both minlex and least_rotation have a key=lambda x: x argument which is then used on the comparisons. It would then be up to the Polyhedron constructor to use minlex with key=default_sort_key, that is minlex(f, directed=False, key=default_sort_key) here

[MarkCBell]

It would be good to add a test for a string, too.

@smichr There is already a doctest for this. Are you suggesting a separate unittest too?

[oscarbenjamin]

Doctests are just for testing the documentation. We should have unit tests to test the correctness of the code.

[MarkCBell]

Doctests are just for testing the documentation. We should have unit tests to test the correctness of the code.

Ok, I've just checked and there is also a unittest already.

[oscarbenjamin]

I thought it was ready to merge. What were the last two commits for?

[MarkCBell]

I thought it was ready to merge. What were the last two commits for?

So following the discussion around whether least_rotation should use default_sort_key I have replaced this with a generic key function (which is sympy.Id by default). Users can then pass in key=default_sort_key if required, and this is now what is done within the Polyhedron method which (unusually) requires this.

I've added a unittest & doctest to demonstrate the use of this.

[oscarbenjamin]

Thanks @MarkCBell and @smichr! This looks good