Polyhedron and combinatorics; evalf(1); quick_sort; Basic.copy() #1508

Merged
merged 190 commits into from Sep 18, 2012

Conversation

Projects
None yet
Member

smichr commented Aug 24, 2012

this is the same as #1498 but committed against 0.7.2

I'll take a look at this as soon as I have some free time : )

Member

smichr commented Aug 24, 2012

Thanks!

Owner

asmeurer commented Aug 24, 2012

Why is this against 0.7.2?

sympy/combinatorics/permutations.py
+ rv.extend([[i] for i in sorted(need)])
+ return rv
+
+def cyclic_form1(cyclic_form0, singletons=False):
@haz

haz Aug 24, 2012

Contributor

Is there any merit to having full_cyclic_form1 and/or cyclic_form0 defined?

@smichr

smichr Aug 25, 2012

Member

It looks like they were added for convenience so when someone wants to enter notation as it appears in a book, for example, that is 1-based with singletons omitted, and see the result in the same form, they can do

p = Permutation(full_cyclic_form1(what_I_see, size))
...do something
print cyclic_form1(p.cyclic_form)

The full_cycle_form0 is also needed because Permutation requires that all indices be present whereas standard notation allows singletons to be omitted. Perhaps methods could be modifed to use a sparse-permutation storage and then it wouldn't matter if they used 1-based or 0-based notation.

sympy/combinatorics/permutations.py
+ min = 1 + (not bool(singletons))
+ return [[e + 1 for e in c] for c in cyclic_form0 if len(c) >= min]
+
+def _af_parity(pi):
@haz

haz Aug 24, 2012

Contributor

Why have all of these functions been stripped of perm? I would think the convention of starting with an underscore (_) doesn't apply here.

@smichr

smichr Aug 25, 2012

Member

These were not really meant for public consumption. They should be stored as class functions inside Permutation but since they can just be imported from Permutations. I left the af part on as a reminder that one must be working with array forms.

sympy/combinatorics/polyhedron.py
+ """
+ Represents the Polyhedral symmetry group.
+
+ It is one of the symmetry groups of the Platonic solids.
@haz

haz Aug 24, 2012

Contributor

"It" --> "The polyhedral symmetry group" (either all caps (P.S.G.) or none)

@smichr

smichr Aug 25, 2012

Member

changed to


    Represents the polyhedral symmetry group (PSG).

    The PSG is one of the symmetry groups of the Platonic solids.

sympy/combinatorics/polyhedron.py
+ # the vertices of the double which sits inside a give polyhedron
+ # can be found by tracking the faces of the outer polyhedron.
+ # A map between face and the vertex of the double is made so that
+ # afater rotation the position of the vertices can be located
@haz

haz Aug 24, 2012

Contributor

"afater" --> "after"

sympy/combinatorics/polyhedron.py
+ (3,4,9,13,8), (0,4,9,14,5),
+ (5,10,16,15,14), (6,10,16,17,11), (7,11,17,18,12),
+ (8,12,18,19,13), (9,13,19,15,14),
+ (15,16,17,18,19)]
@haz

haz Aug 24, 2012

Contributor

There a reason for this odd spacing?

@smichr

smichr Aug 25, 2012

Member

just trying to show the convention for numbering as described in the docstring: top face, next 5 equitorial faces and the 5 below them, and the bottom face. (And the list of indices is too long if they are unwrapped.)

sympy/utilities/iterables.py
@@ -1236,3 +1257,28 @@ def generate_oriented_forest(n):
break
else:
break
+
+def quick_sort(seq, quick=True):
@haz

haz Aug 24, 2012

Contributor

quick doesn't seem to be used at all in this function.

@smichr

smichr Aug 25, 2012

Member

corrected to use default_sort_key when quick is not True

Contributor

haz commented Aug 24, 2012

I haven't been following things too closely, so my feedback is quite shallow unfortunately. One further meta comment I had was about xrange -- I remember seeing that sympy was moving towards using only range. Is this not the case anymore?

Owner

asmeurer commented Aug 24, 2012

xrange vs. range doesn't matter that much. If it's potentially big, use xrange. And obviously if you need to index it, use range.

Member

smichr commented Aug 25, 2012

Why is this against 0.7.2?

Because I was hoping to have it be part of 0.7.2 to fill out the combinatorics changes that were already made.

Member

smichr commented Aug 25, 2012

xrange has been changed to range. question to self: is range indexable in 3.3 or does it return an iterator?

Member

smichr commented Aug 25, 2012

ok, range(10, 20, 2)[3] works in 3.x

Member

smichr commented Aug 25, 2012

There are going to be failures because of the R to L multiplication change. I'll work these out but am leaving this open for any other comments.

Member

smichr commented Aug 25, 2012

I think things are ok after all.

Owner

asmeurer commented Aug 25, 2012

Unless this has a significant API change, it would be better to merge this into master. 0.7.2 should only have bug fixes or major API changes that would rather not wait a release, so that it can stabilize.

Owner

asmeurer commented Aug 25, 2012

Sorry to be a stickler about it, but if we only released when there are no new patches coming in, then we would never release (this is almost what is happening already).

Member

smichr commented Aug 26, 2012

No problem either way, but here's what's happened so you can decide which way you want this to go:

  1. evalf(1) can be done
  2. there is a quick_sort function for handling intra-session arg ordering
  3. Polyhedron has been added
  4. lots of changes to how Permutation works and many of the previous functions deleted

Perhaps 1 and 2 could go in now? We're still discussing the current behavior of how Permutations are multiplied and if a change is made there it's going to be major.

Member

smichr commented Aug 26, 2012

I can close this and reopen 1498. I'll wait for your advice, @asmeurer .

Owner

asmeurer commented Aug 26, 2012

I think if anything we should try to finalize the Permutation convention before the release, especially if we are going to change it. But how soon can it be done?

Member

jrioux commented Aug 27, 2012

What is the benefit of quick_sort, is it really that much faster than sorted(x, key=default_sort_key) and worth the complication?

Member

smichr commented Aug 27, 2012

It uses hash sorting and if there are no clashes, you are done. This has
got to be faster than constructing a key based on the tree of an
expression...at least I would think so. And I guess that's why it is used
in Add to do the sorting (or is it Mul -- one of the two).

Member

smichr commented Aug 27, 2012

I know there is a test failure in the demonstration of using DisjointCycle multiplication because I'm floundering with the class construction. I would like DisjointCycle and BaseDisjointCycle (should be renamed to put Base at the end) to be merged together if possible -- but not sure that it is possible -- and I want the returned object to come back as DisjointCycle, not defaultdict and I'm not sure how to get that done. Can anyone see what I'm trying to do and give me a hand? @ness01 , perhaps, or anyone else that is fluent in object creation?

Contributor

ness01 commented Aug 27, 2012

I'm sorry, but it is unclear to me what you are trying to do. The following merges BaseDisjointCycle and DisjointCycle, and creates the constructor you like, plus copying:

diff --git a/sympy/combinatorics/permutations.py b/sympy/combinatorics/permutations.py
index 5746162..c0ba9c9 100644
--- a/sympy/combinatorics/permutations.py
+++ b/sympy/combinatorics/permutations.py
@@ -213,9 +213,7 @@ def __str__(self):
         reduced = Permutation(self.as_list()).cyclic_form
         return str([tuple(c) for c in reduced])

-
-class DisjointCycle(BaseDisjointCycle):
-    def __new__(cls, *args):
+    def __init__(self, *args):
         """Load up a BaseDisjointCycle instance with the values for the cycle.

         Examples
@@ -226,7 +224,11 @@ def __new__(cls, *args):
         """

         from sympy.ntheory.residue_ntheory import int_tested
-        d = BaseDisjointCycle()
+        if len(args) == 1 and isinstance(args[0], BaseDisjointCycle):
+            for k, v in args[0].iteritems():
+                self[k] = v
+            return
+        d = self
         if not args:
             return d
         args = int_tested(args)
@@ -234,8 +236,9 @@ def __new__(cls, *args):
         args += [args[0]]
         for i in range(n):
             d[args[i]] = args[i + 1]
-        return d

+    def copy(self):
+        return BaseDisjointCycle(self)

 class Permutation(Basic):
     """

Note that it is not obvious to me if this is particularly pythonic style.

In any case, what is the difference between the BaseDisjointCycle class and the Permutation class? Note that the product of two cycles need not be a cycle, so I suppose BaseDisjointCycle is a representation of a permutation in disjoint cycle form, plus multiplication in reversed order? And interpretation of iterables as cycles? If this is the case, I would suggest that the concerns should be separated: (1) a method (not necessarily in the python sense) to reverse multiplication order, (2) a method (in the python sense) to display the disjoint cycles of a permutation, (3) a method (not necessarily in the python sense) to interpret iterables as cycles, and (4) possibly a redesign of the permutation classes to allow different representations, one of which could be via disjoint cycles.

For (1), I would like to use the python with statement if possible. We could additionally support a function leftMul(perm1, perm2, ...). A thin wrapper LeftMul()_perm1_perm2 could also work, but getting out the permutation requires additional brackets as far as I can tell.

(2) Is very easy to implement.

(3) Could either follow the same ideas as (1), or (imho preferrably) just declare tuples to be cyles and lists to be "permutations in array form".

(4) I don't know enough about.

I hope this is of any help. Let me know if you would like me to code up some of this, I should have smallish amounts of free time starting from next week.

Owner

asmeurer commented Aug 28, 2012

(1) would be very painful, because it would have to change the order globally, meaning that all algorithms would essentially have to be written in an order independent way.

Member

smichr commented Aug 28, 2012

btw, making mul work from R to L or L to R is easy to do. We just need to decide how P1*P2 should be interpreted: apply P1 then apply P2 or vice versa. I'm leaning towards making the multiplilcation go from R to L.

Contributor

ness01 commented Aug 28, 2012

@asmeurer Hm true.

@smichr I think it is clear that we can make __mul__ work L-R or R-L, the more interesting question is which of them (or both) to support.

Contributor

ness01 commented Aug 28, 2012

@asmeurer Note that at least for groups, you can essentially switch between left and right multiplication by replacing every element by its inverse. (This isn't practical when entering, but allows an easy wrapper for algorithms, I think.)

Member

smichr commented Aug 28, 2012

Permutations now work from R to L in Permutation and Cycle form. lmul is
the public classmethod to do otherwise: Permutation.lmul(a, b) = b*a

Member

jrioux commented Aug 30, 2012

OK, quick_sort is definitely faster than default_sort_key and is stable regarding hash collisions. Here are some timing, without hash collisions:

47.12 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_1, key=hash)
47.60 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_2, key=hash)
52.52 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_1, cmp=cmp_hash)
53.15 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_2, cmp=cmp_hash)
53.12 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_1, cmp=cmp_hash_with_tiebreaker)
54.28 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x] sorted(ll_2, cmp=cmp_hash_with_tiebreaker)
56.40 usec/pass (cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x) quick_sort(ll_1)
56.29 usec/pass (cos(x), 3, 5, 75, 1442140491547705646, x*y, 5*x) quick_sort(ll_2)
223.06 usec/pass [3, 5, 75, 1442140491547705646, 5*x, x*y, cos(x)] sorted(ll_1, key=default_sort_key)
226.69 usec/pass [3, 5, 75, 1442140491547705646, 5*x, x*y, cos(x)] sorted(ll_2, key=default_sort_key)
310.64 usec/pass [3, 5, 75, 1442140491547705646, 5*x, x*y, cos(x)] sorted(ll_1)
319.67 usec/pass [3, 5, 75, 1442140491547705646, 5*x, x*y, cos(x)] sorted(ll_2)

and with an hash collision:

48.71 usec/pass [cos(x), 3, 5, 75, y, 1442140491547705646, x*y, 5*x] sorted(ll_1, key=hash)
48.65 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x] sorted(ll_2, key=hash)
56.43 usec/pass [cos(x), 3, 5, 75, y, 1442140491547705646, x*y, 5*x] sorted(ll_1, cmp=cmp_hash)
56.48 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x] sorted(ll_2, cmp=cmp_hash)
104.20 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x] sorted(ll_1, cmp=cmp_hash_with_tiebreaker)
105.36 usec/pass [cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x] sorted(ll_2, cmp=cmp_hash_with_tiebreaker)
107.89 usec/pass (cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x) quick_sort(ll_1)
107.68 usec/pass (cos(x), 3, 5, 75, 1442140491547705646, y, x*y, 5*x) quick_sort(ll_2)
233.52 usec/pass [3, 5, 75, 1442140491547705646, 5*x, y, x*y, cos(x)] sorted(ll_1, key=default_sort_key)
237.22 usec/pass [3, 5, 75, 1442140491547705646, 5*x, y, x*y, cos(x)] sorted(ll_2, key=default_sort_key)
429.94 usec/pass [3, 5, 75, 1442140491547705646, y, 5*x, x*y, cos(x)] sorted(ll_1)
360.76 usec/pass [3, 5, 75, 1442140491547705646, y, 5*x, x*y, cos(x)] sorted(ll_2)

The conclusion is that key=hash or cmp=cmp_hash are the fastest but not stable, cmp=cmp_hash_with_tiebreaker (see code below) and quick_sort are performing almost the same and are stable, default_sort_key is slow and sorted used without any key is the slowest.

Here's the code that generated the above:

preamble = """
from sympy import cos, sin
from sympy.abc import x, y

from sympy.utilities import quick_sort
from sympy.utilities.misc import default_sort_key

ll_1 = [y, 5, 3, 75, x*y, hash(y), 5*x, cos(x)]
ll_2 = [hash(y), y, 5*x, 75, 3, y*x, cos(x), 5]

def cmp_hash(a, b):
    return cmp(hash(a), hash(b))

def cmp_hash_with_tiebreaker(a, b):
    return cmp(hash(a), hash(b)) or cmp(default_sort_key(a), default_sort_key(b))

"""

import timeit

exec(preamble)

def test(code):
    t = timeit.Timer(stmt=preamble+code)
    print "%.2f usec/pass" % (1000000 * t.timeit(number=100000)/100000),
    print eval(code),
    print code

test("sorted(ll_1, key=hash)")
test("sorted(ll_2, key=hash)")

test("sorted(ll_1, cmp=cmp_hash)")
test("sorted(ll_2, cmp=cmp_hash)")

test("sorted(ll_1, cmp=cmp_hash_with_tiebreaker)")
test("sorted(ll_2, cmp=cmp_hash_with_tiebreaker)")

test("quick_sort(ll_1)")
test("quick_sort(ll_2)")

test("sorted(ll_1, key=default_sort_key)")
test("sorted(ll_2, key=default_sort_key)")

test("sorted(ll_1)")
test("sorted(ll_2)")
Member

jrioux commented Aug 30, 2012

By the way there are still doc failures:
sympy.combinatorics.permutations.Permutation.get_precedence_distance:15: WARNING: Title underline too short.

Member

smichr commented Aug 30, 2012

Very nice! I like the tie_breaker use of or. But remember that cmp is
deprecated in 3.x

http://www.gossamer-threads.com/lists/python/dev/727798

/c

Member

smichr commented Aug 30, 2012

On Thu, Aug 30, 2012 at 10:49 PM, Julien Rioux notifications@github.comwrote:

By the way there are still doc failures:
sympy.combinatorics.permutations.Permutation.get_precedence_distance:15:
WARNING: Title underline too short.

I'm not sure what needs to be fixed but I am investingating by running the
docs.

Member

smichr commented Aug 30, 2012

I'll reopen #1498 and close this as a pull against 0.7.2

@smichr smichr closed this Aug 30, 2012

Member

jrioux commented Aug 30, 2012

Ah man, cmp is really deprecated? I gave cmp_to_key a try earlier on but it was really slow (I think it ended up computing the default_sort_key for each element).

Owner

asmeurer commented Aug 31, 2012

cmp is by its nature slow. What you want to do is define a key. That way, when sorting, it is computed exactly once for each object.

Member

smichr commented Aug 31, 2012

On Fri, Aug 31, 2012 at 8:39 AM, Aaron Meurer notifications@github.comwrote:

cmp is by its nature slow. What you want to do is define a key. That way,
when sorting, it is computed exactly once for each object.

But what we need is an incremental key that advances complexity only when
necessary (i.e. when there are clashes). If there are no hash collisions
there is no need to compute the default_sort_key for items -- even once.

@smichr smichr reopened this Sep 8, 2012

Member

smichr commented Sep 11, 2012

@asmeurer, it looks like I need to rebase on 0.7.2 but I'm having a hard time finding that branch. I don't see it listed under your branches, only


aaron/0.6.7         aaron/0.7.0-merge   
aaron/0.7.0         aaron/0.7.1         

Could you tell me how to find this, please?

Member

smichr commented Sep 11, 2012

If there are no further comments I will commit this in 24 hours. Thanks for all the feedback everyone!

Member

smichr commented Sep 11, 2012

If there are no further comments I will commit this in 24 hours. Thanks for all the feedback everyone!

Member

smichr commented Sep 11, 2012

@asmeurer , I don't know why the first few commits (like the change to AUTHORS) is showing up. I accidentally rebased on an old 0.7.2 and then re-rebased on the new 0.7.2. I would have thought the old commits would have evaporated in the process.

Member

jrioux commented Sep 11, 2012

If this is intended for the 0.7.2 release branch, at least this commit should be rebased out: Update version to 0.7.2-git 8948ab0

saptman and others added some commits Oct 18, 2011

combinatorics/permutations: Made the permutation object indexable.
Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Added a Polyhedron class.
Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Added tests for Polyhedron.
Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Added tests for make_perm.
Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Fixed the doctest to make things a bit
more clear.

Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Made the test code a bit more
streamlined.

We use a set in edge list construction.

Fixed some doctests comments.

Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Made the docstrings a bit more clearer.
Helped-by: Chris Smith <smichr@gmail.com>
Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
combinatorics/polyhedron: Simplified docstring and added a
reference.

Signed-off-by: Saptarshi Mandal <sapta.iitkgp@gmail.com>
Member

jrioux commented Sep 14, 2012

SymPy Bot Summary: 🔴 Failed after merging smichr/combinatorics (dee3ba2) into origin/0.7.2 (fd2ca07).
@smichr: Please fix the test failures.
✳️ Python 2.7.2-final-0: pass
🔴 Python 3.2.1-final-0: fail
✳️Sphinx 1.1.3: pass
Docs build command: make clean && make html-errors && make clean && make latex && cd _build/latex && xelatex -interaction=nonstopmode sympy-*.tex

Member

smichr commented Sep 14, 2012

I really need some help here:

The args of a PermutationGroup are Permutations

>>> G.args
(Permutation(4)(1, 2), Permutation(4)(1, 2, 3))

The args of a Permutation are tuples

`>>> [g.args for g in G]
[((0, 2, 1, 3, 4),), ((0, 2, 3, 1, 4),)]

Why when I use has() on PermutationGroup do I get the error,

G.has(G[0])
python3.2/lib/python3.2/site-packages/sympy/core/cache.py", line 85, in wrapper
4122    return func_cache_it_cache[k]
4123TypeError: unhashable type: 'PermutationGroup'

It appears that the name itself -- PermutationGroup -- is the unhashable entity.
What does it take to make a type hashable??

Note: in wrapper, in line 80 the type is being entered into the list to prepare the
hash:

k = [(x, type(x)) for x in args]

smichr added some commits Sep 14, 2012

Merge pull request #15 from pernici/combinatorics
Changed is_subgroup to admit that a group can be subgroup of a group with larger degree
Contributor

pernici commented Sep 14, 2012

>>> from sympy.combinatorics.permutations import Permutation
>>> from sympy.combinatorics.perm_groups import PermutationGroup
>>> from sympy.combinatorics.named_groups import SymmetricGroup, CyclicGroup
>>> S5 = SymmetricGroup(5)
>>> G = CyclicGroup(7)*S5
>>> G5 = PermutationGroup([Permutation(x, size=G.degree) for x in S5])
>>> S5.generators
[Permutation(0, 1, 2, 3, 4), Permutation(4)(0, 1)]
>>> G5.generators
[Permutation(11)(0, 1, 2, 3, 4), Permutation(11)(0, 1)]
>>> G.generators
[Permutation(11)(0, 1, 2, 3, 4, 5, 6), Permutation(7, 8, 9, 10, 11), Permutation(11)(7, 8)]
>>> S5.is_subgroup(G)
False
>>> G5.is_subgroup(G)
False

G5 elements are S5 elements padded on the right, in array form,
with range(S5.size, G.size), that is g5_el = s5_el.direct_product(range(7))' Thedirect_product` method is missing in permutations.py; cfr. DirectProduct

>>> G1 = S5*CyclicGroup(7);
>>> G1.generators
[Permutation(11)(0, 1, 2, 3, 4), Permutation(11)(0, 1), Permutation(5, 6, 7, 8, 9, 10, 11)]
>>> G5.is_subgroup(G1)
True

Compare with GAP

gap> S5 := Group((1,2,3,4,5),(1,2));
gap> G := Group((1,2,3,4,5,6,7), (8,9,10,11,12), (8,9));
gap> IsSubgroup(G, S5);
false
gap> G1 := Group((1,2,3,4,5), (1,2),(6,7,8,9,10,11,12));
Group([ (1,2,3,4,5), (1,2), (6,7,8,9,10,11,12) ])
gap> IsSubgroup(G1, S5);
true

or Sage

sage: S5 = PermutationGroup([(1,2,3,4,5), (1,2)])
sage: G = PermutationGroup([(1,2,3,4,5,6,7), (8,9,10,11,12), (8,9)])
sage: sage: S5.is_subgroup(G)
False
sage: S5.is_subgroup(G1)
True

Notice that in Gap permutations are in cycle form with no size indication,
so there is no difference between S5 and G5; similarly in the Sage example.

I have made a pull request smichr#15 so that is_subgroup behaves
as Sage in this example.

Member

smichr commented Sep 14, 2012

So is is_subgroup working as it ought or do we need a fuzzy_eq method that compares them in some way that accounts for size?

Contributor

pernici commented Sep 14, 2012

I modified is_subgroup to allow the subgroup H to have smaller degree, as in the examples I gave;
the idea is that one looks only at the cyclic_form , not at the size.
I am not entirely convinced that this behavior is a good idea;
I do not see why SymmetricGroup(3).is_subgroup(SymmetricGroup(3)*CyclicGroup(5)) should be True,
while SymmetricGroup(3).is_subgroup(CyclicGroup(5)*SymmetricGroup(3)) should be False;
in both cases SymmetricGroup(3) is isomorphic to a subgroup of G.

Notice that I allowed this behavior only when H has degree not larger than G,
H.is_subgroup(G) returns False even in the following case, in which G3 and SymmetricGroup(3)
have the generators with the same cyclic_form

>>> from sympy.combinatorics.permutations import Permutation
>>> from sympy.combinatorics.perm_groups import PermutationGroup
>>> from sympy.combinatorics.named_groups import SymmetricGroup
>>> G3 = PermutationGroup([Permutation(x, size=10) for x in SymmetricGroup(3)])
>>> G3.is_subgroup(SymmetricGroup(4))
False
>>> SymmetricGroup(3).is_subgroup(SymmetricGroup(4))
True

I would not know how to use def fuzzy_eq(p1, p2): return p1.cyclic_form == p2.cyclic_form
in the code for is_subgroup

Owner

asmeurer commented Sep 14, 2012

SymPy Bot Summary: ✳️ Passed after merging smichr/combinatorics (5f3cbe87529bc3948c135988a3182e102d336956) into origin/0.7.2 (fd2ca07).
✳️ Python 2.7.3-final-0: pass

Owner

asmeurer commented Sep 14, 2012

I can't reproduce the has issue. I deleted your has and tried

In [10]: G = PermutationGroup(*(Permutation(4)(1, 2), Permutation(4)(1, 2, 3)))

In [11]: G.has(G[0])
Out[11]: True
Member

smichr commented Sep 15, 2012

has problems

the problem is only with Python 3

Member

smichr commented Sep 15, 2012

@pernici , it would be nice to have a good working definition of what it means to be a subgroup. e.g I find the following online:


Subgroup

If S is a subset of the group G (i.e. a subset of the set of elements of G) then we 
say S is a subgroup if it is also a group under the operation of G.

A subset S of a group G may fail to be a subgroup in a few different ways; 
here are examples. If S does not contain the identity element, it violates 
one of the criteria in the definition of a group. If S contains an element but 
not that element's inverse, it would violates another of those criteria. If S 
contains two elements but not their product, then the binary operation of G 
cannot really be said to be a binary operation on S, because it maps some 
pairs from S outside of S.

The problem with that definition is that the conditions have nothing to do with G; they are the generic conditions of what it means to be a group. And I've already defined a function to check for that: is_group. Regarding that, I perhaps could simplify it by using the 'one test' method referred to here:


From: Doctor Paul
Subject: Re: Abstract algebra - groups

To show that a subset H of a group G is a subgroup, you need to show 
three things:

1. that the identity element of G is in H.

2. that if x is in H then x^-1 is in H.

3. that if x and y are in H then x*y is in H.

There is a shorter way to show that H is a subgroup. Sometimes this 
shorter way is referred to as the "One step subgroup test."  The 
reason for the name is because you only have to show one thing and it 
is equivalent to the three conditions above.

1.  If x and y are in H, then x*y^-1 is in H.

This page might be useful.

Member

smichr commented Sep 15, 2012

Without a clear definition of is_subgroup I'm shooting in the dark, but here's what I would consider for fuzzy_eq -- but have no idea if this meainingful or not:


def fuzzy_eq(a, b):
    if a.size > b.size:
        a, b = b, a
    a = a.cyclic_form
    b = b.cyclic_form
    if len(a) != len(b):
        return False
    m = min([c[0] for c in b])
    n = len(a)
    return all(e[0] == e[1] - m for i in range(n) for e in zip(a[i], b[i]))

def is_sg(self, other):
    return all(any(fuzzy_eq(s, o) for o in other) for s in self)


>>> G, H = SymmetricGroup(3),(CyclicGroup(5)*SymmetricGroup(3))
>>> is_sg(G, H)
True
is_sg(H, G)
True

>>> G3 = PermutationGroup([Permutation(x, size=10) for x in SymmetricGroup(3)])
>>> is_sg(G3, SymmetricGroup(4))
True

Owner

asmeurer commented Sep 15, 2012

The problem with that definition is that the conditions have nothing to do with G; they are the generic conditions of what it means to be a group.

That's because the concept of sub____ is defined for any algebra (set with operations defined on it). So the definition of subring, subfield, subspace (for vector spaces), submodule, subgroup, etc. are all exactly the same. A is a subwhatever of B if:

  1. A is a subset of B.
  2. A is closed under the algebraic operations defined on B.

For groups, this means closure under group multiplication, inverses, and identity (inverse and identity are considered uniary and nullary operations on G). This generic definition is useful from the viewpoint of universal algebra, but if you are just talking about groups, it's usually clearer to state it as, "If G is a group, the H is a subset of G iff:

  1. H is a subset of G.
  2. The group operation on H is the same as the group operation on G (restricted to H). This means that multiplications and inverses in H should give the same thing as in G. Also, the identity in H should be the same as the identity in G.
  3. H is closed under the group operation on G (if x and y are in H, then x*y is in H).
  4. H is closed under inverses (if x is in H, then x**-1 is in H)."

One of your two definitions may be clearer than what I just wrote. The first one is very terse, which may or may not be nice (the details are hidden in the statement "is a group", which implies closure under the operations).

The x*y**-1 condition is indeed useful for proving that a subset is a subgroup, but it's not suitable as a definition. Rather, the definition should be the one given above, which generalizes to any algebra.

One final note: the definition of subgroup also contains an additional caveat that I didn't mention above (and isn't mentioned in either of your definitions). The empty set is never a subalgebra.

Does that clear things up, or just make things more confusing?

Owner

asmeurer commented Sep 15, 2012

The Python 3 issue is indeed strange. The issue is not really has. It's hash(G) (this causes problems in has because of the cache). For some reason, G.__hash__ doesn't seem to be inherited from Basic. This is going to require some searching.

Owner

asmeurer commented Sep 15, 2012

Ah, here we go, from http://docs.python.org/dev/reference/datamodel.html#object.__hash__.

If a class that overrides __eq__() needs to retain the implementation of __hash__() from a parent class, the interpreter must be told this explicitly by setting __hash__ = <ParentClass>.__hash__. Otherwise the inheritance of __hash__() will be blocked, just as if __hash__ had been explicitly set to None.

So the problem comes from your redefinition of __eq__. So you either need to remove that or define

def __hash__(self):
    return super(PermutationGroup, self).__hash__()

Perhaps we should document somewhere that redefining _hashable_content is not enough if you also redefined __eq__.

smichr added some commits Sep 14, 2012

fix trouble with hash and PermutationGroup
The redefining of __eq__ required that __hash__ also be defined
as noted now in the __eq__ docstring that was edited in Basic.
Member

smichr commented Sep 15, 2012

Big thanks, @asmeurer ! I added the notes to Basic's docstring about this and fixed PermutationGroups.

Member

smichr commented Sep 15, 2012

does that help

A bit, but in the function that we've written, we are seeing if all permutations in self are contained in the group of interest (i.e. can be written as the product of permutations in the stabilizer cosets). We don't test that those elements are a group so we don't apply the additional constraints that are part of what it means to be a group.

btw, __eq__ is not in keeping with the general sympy definition of __eq__. Right now, it is a non-resizing equivalent of is_subgroup. At most __eq__ should just be a test whether the set of generators of A and B are the same. A new function replacing __eq__ should be defined. I tried to make the change but it's not trivial.

Contributor

pernici commented Sep 15, 2012

it would be nice to have a good working definition of what it means to be a subgroup

The problem here is which is the definition of group.

For us a PermutationGroup of degree n is defined in terms of
permutations acting on the domain [0,..,n-1]

S3 = SymmetricGroup(3) and
G3 = PermutationGroup([Permutation(x.array_form, n) for x in S3.generators])

with n > 3 S3andG3` are groups with different degrees, therefore they are different.

In GAP a permutation group is defined on the points which it does not keep
fixed, that is on its support.

Therefore there is no distinction between S3 and G3, so that in
particular S3 is a subgroup of G3.

A similar problem presents in the case of is_transitive and degree

>>> from sympy.combinatorics.named_groups import SymmetricGroup
>>> S4 = SymmetricGroup(4)
>>> H = S4.stabilizer(2)
>>> H.is_transitive

while in GAP

gap> S4 := SymmetricGroup(4);
gap> H := Stabilizer(S4, 3);
gap> IsTransitive(H);
true
gap> DegreeAction(H);
3

I will write a PR in which default arguments are added to is_subgroup, is_transitive and degree
for the case in which only the action on the support is considered.

Member

smichr commented Sep 15, 2012

I found the problem that was holding me up. __eq__ is now a nearly trivial test of equality; the former code has been moved into contains. Is it always true that if A.contains(B) that B.contains(A) -- in non-sympy, is it always true that if A's permutations can be written as products of B's coset elements that B can be written as A's coset permutations? No test says otherwise and I left commits so that I could undo that assumption if I need to.

Member

smichr commented Sep 15, 2012

Ignore the previous question about A and B and contains. I moved the old __eq__ code to is_in and this can be renamed easily if necessary. I wonder if is_subgroup and is_in can be merged, perhaps by adding a keyword like strict which, if False, will do the resizing that is_subgroup does. I feel pretty confident that we shouldn't have __eq__ doing what it used to be doing, even if it means losing the convenience of using ==. Other ideas?

Member

smichr commented Sep 15, 2012

@pernici , this should be ready for any modifications that you want to make.

Contributor

pernici commented Sep 16, 2012

After the change in __eq__ to check that two groups have the same elements but have different generators
one can do G.is_subgroup(H) and G.order() == H.order(), which is longer than G == H

Member

smichr commented Sep 16, 2012

is_subgroup includes the check that

if G.order() % self.order() != 0:
            return False

If strict is True, should that be G.order() != self.order(), hence

if strict and G.order() != self.order or G.order() % self.order() != 0:
    return False
Owner

asmeurer commented Sep 16, 2012

Shouldn't that be if strict and G.order() == self.order or G.order() % self.order() != 0:

Contributor

pernici commented Sep 16, 2012

It seems to me that the definition of is_subgroup is correct;
when strict and G.order() != self.order , self can be a subgroup of G, for instance A_5 is a subgroup of
S_5, with S_5.order() = 120, A_5.order() = 60

Contributor

pernici commented Sep 16, 2012

I made a pull request smichr#16; apart from adding a default argument to
is_transitive, I fixed a bug in base; the order in the strong base matters, but I used previously a dict
in computing it, so the order was in general lost.

Merge pull request #16 from pernici/combinatorics
Added default argument `strict` to is_transitive`; fixed bug in `base`
Member

smichr commented Sep 17, 2012

is_subgroup correct

I think I was confusing degree with order. I think the current implementation is fine.

I fixed the doctest error in the patch and made another modification.

SymmetricGroup(3).is_subgroup(CyclicGroup(5)*SymmetricGroup(3)) should be True

In order for this to happen we need better resizing. For this case, Permutation(*range(n)) should be resized to Permutation(*range(new_n)) and Permutation(*range(n-1)) resized to Permutation(*range(new_n-1)) BUT how do we know that that is what is intended by a given permutation, what if the user meant "a cycle of only the first two elements" not "a cycle of all but the last element". Is resizing ever a good idea? I'm leaning towards "no" (even though I just added another commit for it that allows resizing in contains()).

Contributor

pernici commented Sep 17, 2012

SymmetricGroup(3).is_subgroup(CyclicGroup(5)*SymmetricGroup(3)) should be True

No, I wrote that before, but it is not true.
SymmetricGroup(3) acts on [0,1,2], while the subgroup SymmetricGroup(3)
in CyclicGroup(5)*SymmetricGroup(3) has support [5,6,7] (it leaves fixed 1,..,4), so they are different groups,
although isomorphic.
Different is the case of

>>> SymmetricGroup(3).is_subgroup(SymmetricGroup(3)*CyclicGroup(5), strict=False)
True

In this case the two SymmetricGroup(3) have the same support, so in the strict=False sense they are equal.

Contributor

pernici commented Sep 17, 2012

I would like to eliminate the stabilizer_cosets method; it is almost a duplicate of basic_transversals;
_stabilizer_cosets can be trivially obtained from _transversals and _base; if i is not in G._base
then G._stabilizer_cosets[i] = [range(size)]; otherwise G._stabilizer_cosets[i] is the list of the array forms
of G._transversals[i].

For instance

>>> G = PermutationGroup([Permutation(6)(0,1,2), Permutation(5,6)])
>>> G.base
[0, 5]
>>> G.stabilizer_cosets()[0]
[[0, 1, 2, 3, 4, 5, 6], [1, 2, 0, 3, 4, 5, 6], [2, 0, 1, 3, 4, 5, 6]]
>>> G.basic_transversals[0]
{0: Permutation([], size=7), 1: Permutation([1, 2, 0], size=7), 2: Permutation([2, 0, 1], size=7)}
>>> G.stabilizer_cosets()[1]
[[0, 1, 2, 3, 4, 5, 6]]
Member

smichr commented Sep 17, 2012

On Mon, Sep 17, 2012 at 11:44 AM, pernici notifications@github.com wrote:

SymmetricGroup(3).is_subgroup(CyclicGroup(5)*SymmetricGroup(3)) should be
True

No, I wrote that before, but it is not true.
SymmetricGroup(3) acts on [0,1,2], while the subgroup SymmetricGroup(3)
in CyclicGroup(5)*SymmetricGroup(3) has support [5,6,7](it leaves fixed 1
,..,4), so they are different groups,
although isomorphic.
Different is the case of

OK. So do you think this is ready to commit to 0.7.2 or is there something
else that you think should be modified?

Contributor

pernici commented Sep 17, 2012

In smichr#17 coset_factor is 2x faster; I have taken away

        # look for another quick exit
        if any(p == g for ui in u for p in ui):
            return [I]*(len(u) - 1) + [g]

because g does not always belong to the last coset representative.

Contributor

pernici commented Sep 17, 2012

OK. So do you think this is ready to commit to 0.7.2 or is there something else that you think should be modified?

I do not like the duplication of data between _stabilizer_cosets and _transversals, but I suppose it can be
eliminated in some later PR.
I think it is ready to be committed.

Member

smichr commented Sep 18, 2012

OK, this is in. Thanks to all who helped in the review process. This ended up being a lot more involved that I imagined.

smichr added a commit that referenced this pull request Sep 18, 2012

Merge pull request #1508 from smichr/combinatorics
Polyhedron and combinatorics; evalf(1); quick_sort; Basic.copy()

@smichr smichr merged commit 49e26fb into sympy:0.7.2 Sep 18, 2012

1 check passed

default The Travis build passed
Details
Owner

asmeurer commented Sep 18, 2012

Awesome. Hopefully no issues slipped past Travis. It's time to release...

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:0.7.2*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:0.7.2*.

Coverage Status

Changes Unknown when pulling 419d310 on smichr:combinatorics into * on sympy:0.7.2*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment