``FreeGroup`` implementation in SymPy #10350

Merged
merged 32 commits into from May 20, 2016

Projects

None yet

3 participants

@gxyd
Member
gxyd commented Jan 2, 2016

Implementation of FreeGroups in SymPy, the way in which the output is produced resonates very much with the way in GAP.

  1. I have not added the tests. But the doctests would serve the purpose, since they pass the tests.
  2. Some of the important things like subgroup still need to be added and more infact..
  3. I closed the previous PR #10263 since i think this PR's approach looks good to me.(i did worse in previous PR to even think of using symbols at some places i think).

ping @jksuom

@gxyd gxyd changed the title from ``FreeGroup`` implementation in SymPy to [WIP] ``FreeGroup`` implementation in SymPy Jan 2, 2016
@jksuom jksuom commented on an outdated diff Jan 2, 2016
sympy/combinatorics/free_group.py
+ """
+ Since any number of arguments can be passed in the form of string,
+ hence `rank` is "not" used as argument.
+ """
+
+ obj = Basic.__new__(cls, *args, **kwargs)
+
+ if isinstance(args[0], int) and args[0] >= 0:
+
+ # (1) form of the API used here
+ if len(args) == 1:
+ obj._str = "f"
+
+ # (2) form of the API used here
+ elif len(args) == 2:
+ obj._str = args[1]
@jksuom
jksuom Jan 2, 2016 Member

Should the type of args[1] be checked?

@gxyd
Member
gxyd commented Jan 2, 2016

Also one thing that currently does not work is the way simplification occurs in GAP.
Currently

>>> f = FreeGroup( 4 )
>>> (f[0]*f[1])**2
f[0]*f[1]*f[0]*f[1]         # better represented as (f[0]*f[1])**2  i.e no expansion

I will next try to see this point.

@jksuom jksuom commented on an outdated diff Jan 2, 2016
sympy/combinatorics/free_group.py
+ Examples
+ ========
+
+ >>> from sympy import FreeGroup
+ >>> f = FreeGroup( 4, "swapnil" )
+ >>> g = FreeGroup( 4, "swapnil" )
+ >>> f == g
+ False
+ >>> f == f
+ True
+
+ """
+ if self is other:
+ return True
+ else:
+ return False
@jksuom
jksuom Jan 2, 2016 Member

Perhaps return self is other would suffice, and it is probable that Python would provide this as a default. (From the library manual: "Non-identical instances of a class normally compare as non-equal unless the class defines the __eq__() method or the __cmp__() method.")

@jksuom jksuom commented on an outdated diff Jan 2, 2016
sympy/combinatorics/free_group.py
+
+ """
+ if not isinstance(i, FreeGroupElm):
+ return False
+ elif self != i.group:
+ return False
+ else:
+ return True
+
+ def is_subgroup(self, F):
+ """Return True if all elements of `self` belong to `F`.
+
+ Examples
+ ========
+ """
+ if F.is_group and set(F.generators).issubset(set(self.generators)):
@jksuom
jksuom Jan 2, 2016 Member

I think this should be something like all([self.contains(gen) for gen in F.generators]).

@jksuom jksuom commented on an outdated diff Jan 2, 2016
sympy/combinatorics/free_group.py
+ @property
+ def expt(self):
+ return self._expt
+
+ @property
+ def is_Identity(self):
+ if self.array_form == list():
+ return True
+ else:
+ return False
+
+ @property
+ def array_form(self):
+ """
+ SymPy provides two different internal kinds of representation
+ of assosciative words. The first one is called the `array_form`
@jksuom
jksuom Jan 2, 2016 Member

typo: assosciative

@gxyd gxyd and 2 others commented on an outdated diff Jan 3, 2016
sympy/combinatorics/free_group.py
+ def __eq__(self, other):
+ """No ``FreeGroup`` is equal to any "other" ``FreeGroup``.
+
+ Examples
+ ========
+
+ >>> from sympy import FreeGroup
+ >>> f = FreeGroup( 4, "swapnil" )
+ >>> g = FreeGroup( 4, "swapnil" )
+ >>> f == g
+ False
+ >>> f == f
+ True
+
+ """
+ return self is other
@gxyd
gxyd Jan 3, 2016 Member

it is probable that Python would provide this as a default

But for the above doctest i have added. It is such that even if the two instances f and g have the same arguments (i.e "4" and "swapnil") but still f == g has to be returned as being not equal, different from the usual way Python defines __eq__(which returns True when the arguments are same for two instances of same class).

@jksuom
jksuom Jan 3, 2016 Member

I see... This behaviour is probably inherited from Basic. I wonder if it is necessary to make Group a subclass of Basic. (PolyRing is not...) @asmeurer ?

@asmeurer
asmeurer Jan 3, 2016 Member

Why do you want this behavior to work this way? The whole idea behind Basic.__eq__ is that == works as structural (not mathematical) equality, which tends to be the most useful thing when dealing with symbolic objects.

@gxyd
gxyd Jan 4, 2016 Member

Then, did we made a mistake here in PR #10230 for the PermutationGroup?

@asmeurer
Member
asmeurer commented Jan 3, 2016

I'm not a fan of string based APIs. It's better to create a GroupSymbol object, than to use strings. Otherwise you will always have annoying indirection, since you can't actually multiply strings together.

@asmeurer asmeurer and 1 other commented on an outdated diff Jan 3, 2016
sympy/combinatorics/free_group.py
+ generators are printed as `name0`, `name1` etc., that is, each name is the
+ concatenation of the string `name` and an integer from `0` to `range-1`.
+ The default for `name` is the string "f".
+
+ ``FreeGroup( rank )`` .......................................... (1)
+
+ Called in the second form, ``FreeGroup`` returns a free group on as many
+ generators as arguments, printed as `name0`, `name1` etc.
+
+ ``FreeGroup( rank, "name" )`` ...................................... (2)
+
+ Called in the third form, ``FreeGroup`` returns a free group on as many
+ generators as the length of the list `names`, the i-th generator being
+ printed as `names[i]`.
+
+ ``FreeGroup( "string0", "string1", "string2", ... )`` .............. (3)
@asmeurer
asmeurer Jan 3, 2016 Member

Is this literally with an ellipsis literal (in Python 3)?

@gxyd
gxyd May 11, 2016 Member

No, this is not. I will remove this.

@gxyd
Member
gxyd commented Jan 4, 2016

I'm not a fan of string based APIs.

I too thought the same, hence i left the work for 3rd API form that i have mentioned.

@jksuom jksuom and 1 other commented on an outdated diff May 9, 2016
sympy/combinatorics/free_group.py
+ """
+ is_Identity = None
+ is_AssocWord = True
+
+ def __new__(cls, free_group, array_form, str_expr=""):
+ obj = Basic.__new__(cls, free_group, array_form, str_expr)
+
+ # `array_form` is used internally by the methods
+ # of `FreeGroupElm`
+ obj._group = free_group
+ obj._array_form = array_form
+ obj._str_expr = str_expr
+ return obj
+
+ @property
+ def group(self):
@jksuom
jksuom May 9, 2016 Member

Is there some reason why this could not be an attribute? Maybe even a class attribute.

@gxyd
gxyd May 10, 2016 Member

This attribute of group will vary according to the Free Group element, each FreeGroupElm will have a different Free Group, which will be provided to __new__(cls, free_group, ... ) just above this code.

@gxyd
gxyd May 10, 2016 Member

This can be an instance attribute, but not a class attribute.

@jksuom jksuom commented on an outdated diff May 9, 2016
sympy/combinatorics/free_group.py
+ # not possi
+ new_array.append((a[i] -1, n))
+ else:
+ new_array.append((a[i] - 1, 1))
+ return new_array
+ elif a[i] == a[i + 1]:
+ n += 1
+ else:
+ new_array.append((a[i] - 1, n))
+ n = 1
+
+
+def zero_simp(array_form):
+ for i in range(len(array_form) - 1, -1, -1):
+ if array_form[i][1] == 0:
+ array_form[i: i + 1] = []
@jksuom
jksuom May 9, 2016 Member

There is also del array_form[i] that I find more readable.

@jksuom jksuom commented on an outdated diff May 9, 2016
sympy/combinatorics/free_group.py
+ while j > 0 and len(app) > 0 and l[j - 1] == app[0]:
+ s = l[j] + app[1]
+ if s == 0:
+ j = j - 2
+ else:
+ l[j] = s
+ app = app[2: len(app)]
+
+ if j + 1 < len(l):
+ l = l[0: j + 1]
+
+ if len(app) > 0:
+ l.append(tuple(app))
+ zero_simp(l)
+ mult_simp(l)
+ zero_simp(l)
@jksuom
jksuom May 9, 2016 Member

After the second zero_simp(l) another mult_simp(l) may be needed. Perhaps those functions should be combined.

@gxyd
Member
gxyd commented May 12, 2016

I'm not a fan of string based APIs. It's better to create a GroupSymbol object, than to use strings. Otherwise you will always have annoying indirection, since you can't actually multiply strings together.

Though it is not fully developed at all. I am also in favour of removing that. Now, should i remove that? (this comment was made a while back, so making sure no un-necessary changes). This will change API of FreeGroup from *args, **kwargs to rank, str_expr.

@jksuom jksuom commented on an outdated diff May 12, 2016
sympy/combinatorics/free_group.py
@@ -0,0 +1,872 @@
+from __future__ import print_function, division
+
+from sympy.core.basic import Basic
+from sympy.core.compatibility import as_int
+from sympy.core.sympify import CantSympify
+from mpmath import isint
+from collections.abc import Sequence
@jksuom
jksuom May 12, 2016 Member

collections.abc does not seem to exist in 2.7. There is plain abc, but it does not contain Sequence. Maybe this is not necessary.

@jksuom jksuom commented on an outdated diff May 13, 2016
sympy/combinatorics/free_group.py
@@ -116,11 +110,11 @@ def generators(self):
[swapnil0, swapnil1, swapnil2]
"""
+ group = self
@jksuom
jksuom May 13, 2016 Member

I believe this could be omitted if you write ``def _generators(group):`.

@jksuom jksuom commented on an outdated diff May 14, 2016
sympy/combinatorics/free_group.py
>>> f.generators
- [swapnil0, swapnil1, swapnil2]
+ [x, y, z]
"""
gens = []
for i in range(group.rank):
@jksuom
jksuom May 14, 2016 Member

Maybe for sym in group.symbols:.

@jksuom
Member
jksuom commented May 14, 2016

Looks good. I commented the changes, but it seems to have become 'outdated' (at least in my browser).

@gxyd
Member
gxyd commented May 14, 2016

I have the removed the unused import of collections.abc 😄

@gxyd
Member
gxyd commented May 17, 2016 edited

I have added the tests, added a few more methods. Also the _parse_symbols is now directly used only by the FreeGroup class, instead of being used individually by the vfree_group or free_group etc.
There are a few changes that result due to the use of (symbol, exp) instead of (index, exp) , this include index + some_number gives the next generator while so is not the case with (symbol, exp).
Only a few more tests are needed.

@jksuom jksuom commented on the diff May 17, 2016
sympy/combinatorics/free_group.py
@@ -287,6 +293,14 @@ class FreeGroupElm(CantSympify, tuple):
is_identity = None
is_AssocWord = True
+ _hash = None
+
+ def __hash__(self):
+ _hash = self._hash
+ if _hash is None:
@jksuom
jksuom May 17, 2016 Member

Can this really happen?

@gxyd
gxyd May 17, 2016 edited Member

Whenever an instance of FreeGroupElm is created, initially it has _hash = None (see the assignment just above this), so _hash is None will be True. I don't see anything bad with this check.

@jksuom
jksuom May 17, 2016 Member

Neither do I. Actually, I did not notice that this was for FreeGroupElm and not for FreeGroup.

@gxyd
Member
gxyd commented May 17, 2016

I have made some docstring, printing fixes. The printing fix is a little hacky, though. Apart from that, I think docstrings are complete now.

@gxyd
Member
gxyd commented May 18, 2016

I think this is now ready for a final review.

@jksuom
Member
jksuom commented May 18, 2016

I think this is ready to be merged.

@gxyd gxyd changed the title from [WIP] ``FreeGroup`` implementation in SymPy to ``FreeGroup`` implementation in SymPy May 18, 2016
@gxyd
Member
gxyd commented May 20, 2016

ping @jksuom @asmeurer . Let me know before anyone merges this. I will squash the commits. Next will start with "FreeAbelianGroup", "Coset Enumeration".

@jksuom jksuom merged commit c64194e into sympy:master May 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gxyd gxyd deleted the gxyd:implementation_FreeGroup branch May 20, 2016
@gxyd gxyd added the GSoC label May 20, 2016
@asmeurer asmeurer commented on the diff May 21, 2016
sympy/combinatorics/free_group.py
+
+ >>> from sympy.combinatorics.free_group import free_group
+ >>> F, x, y, z = free_group("x, y, z")
+ >>> F
+ <free group on the generators (x, y, z)>
+ >>> x**2*y**-1
+ x**2*y**-1
+ >>> type(_)
+ <class 'sympy.combinatorics.free_group.FreeGroupElm'>
+
+ """
+ _free_group = FreeGroup(symbols)
+ return (_free_group,) + tuple(_free_group.generators)
+
+@public
+def xfree_group(symbols):
@asmeurer
asmeurer May 21, 2016 Member

I know this is already merged, but what's the point of this separate function?

@gxyd
gxyd May 21, 2016 Member

specifically the xfree_group could be used for collecting the generators of free group in a single variable, directly. Similarly the next function vfree_group could be use to pollute the environment with the generator variables automatically. I don't say we can't live without it. But its good to have it.

@asmeurer asmeurer commented on the diff May 21, 2016
sympy/combinatorics/free_group.py
+ return _symbols(symbols, seq=True)
+ elif isinstance(symbols, Expr):
+ return (symbols,)
+ elif is_sequence(symbols):
+ if all(isinstance(s, string_types) for s in symbols):
+ return _symbols(symbols)
+ elif all(isinstance(s, Expr) for s in symbols):
+ return symbols
+ raise ValueError("")
+
+
+##############################################################################
+# FREE GROUP #
+##############################################################################
+
+_free_group_cache = {}
@asmeurer
asmeurer May 21, 2016 edited Member

Why do you need this, and why don't you use the SymPy cache? Unbounded caches can be problematic.

@asmeurer asmeurer commented on the diff May 21, 2016
sympy/combinatorics/free_group.py
+ else:
+ str_form = "<free group on the generators "
+ gens = self.generators
+ str_form += str(gens) + ">"
+ return str_form
+
+ __repr__ = __str__
+
+ def __getitem__(self, index):
+ symbols = self.symbols[index]
+ return self.clone(symbols=symbols)
+
+ def __eq__(self, other):
+ """No ``FreeGroup`` is equal to any "other" ``FreeGroup``.
+ """
+ return self is other
@asmeurer
asmeurer May 21, 2016 Member

Why this? It seems the usual SymPy definition of __eq__ would be more useful.

@gxyd
gxyd May 21, 2016 Member

This could be changed. Its not something under a priority, so I left it as it is.

@asmeurer asmeurer commented on the diff May 21, 2016
sympy/combinatorics/free_group.py
+ "be multiplied")
+ return self*(other.inverse())
+
+ def __rdiv__(self, other):
+ group = self.group
+ if not isinstance(other, group.dtype):
+ raise TypeError("only FreeGroup elements of same FreeGroup can "
+ "be multiplied")
+ return other*(self.inverse())
+
+ __truediv__ = __div__
+
+ __rtruediv__ = __rdiv__
+
+ def __add__(self, other):
+ raise TypeError("unsupported operand type(s) for + or add: '%s' and '%s'"
@asmeurer
asmeurer May 21, 2016 Member

The correct behavior here (and possibly in other cases where you're raising TypeError) is to return NotImplemented. That way, other classes can define __add__ and __radd__ to add FreeGroupElm objects.

@gxyd
gxyd Jun 11, 2016 Member

Addressed in "coset enumeration" PR #11140 .

@asmeurer asmeurer commented on the diff May 21, 2016
sympy/combinatorics/free_group.py
+ """Return True if all elements of `self` belong to `F`.
+ """
+ return F.is_group and all([self.contains(gen) for gen in F.generators])
+
+ def center(self):
+ """Returns the center of the free group `self`.
+ """
+ return set([self.identity])
+
+
+############################################################################
+# FreeGroupElm #
+############################################################################
+
+
+class FreeGroupElm(CantSympify, DefaultPrinting, tuple):
@asmeurer
asmeurer May 21, 2016 Member

I would spell out FreeGroupElement.

@gxyd
gxyd May 21, 2016 Member

That would be fine.

@gxyd
gxyd Jun 11, 2016 Member

Addressed in "coset enumeration" PR #11140 .

@asmeurer asmeurer commented on the diff May 21, 2016
sympy/printing/pretty/pretty.py
@@ -1715,6 +1715,9 @@ def _print_PolyRing(self, ring):
def _print_FracField(self, field):
return prettyForm(sstr(field))
+ def _print_FreeGroupElm(self, elm):
+ return prettyForm(str(elm))
@asmeurer
asmeurer May 21, 2016 Member

Actual pretty printing would be nice. But for that, you may need to be using Expr or Basic subclasses.

@gxyd gxyd added a commit to gxyd/sympy that referenced this pull request May 23, 2016
@gxyd gxyd ``FreeAbelianGroup`` implementation in SymPy
Implement FreeAbelianGroup and address the remaining issues
commented by Aaron in PR #10350
0797631
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment