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

Added method to calculate Abelian Invariants #16670

Merged
merged 11 commits into from May 13, 2019

Conversation

Projects
None yet
4 participants
@divyanshu132
Copy link
Member

commented Apr 17, 2019

Brief description of what is fixed or changed

Computation of Abelian Invariants has been implemented for both Permutation and finite fp group.

Other comments

Release Notes

  • combinatorics
    • added method for Abelian Invariants computation
@sympy-bot

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hi, I am the SymPy bot (v147). 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:

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

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

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

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
Computation of Abelian Invariants has been implemented for both Permutation and finite fp group.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
*  combinatorics
   * added method for Abelian Invariants computation
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov

This comment has been minimized.

Copy link

commented Apr 17, 2019

Codecov Report

Merging #16670 into master will increase coverage by 0.041%.
The diff coverage is 94.736%.

@@              Coverage Diff              @@
##            master    #16670       +/-   ##
=============================================
+ Coverage   73.805%   73.847%   +0.041%     
=============================================
  Files          619       619               
  Lines       159230    159633      +403     
  Branches     37370     37465       +95     
=============================================
+ Hits        117521    117885      +364     
- Misses       36273     36289       +16     
- Partials      5436      5459       +23
@@ -117,6 +117,8 @@ class PermutationGroup(Basic):
.. [13] http://www.math.colostate.edu/~hulpke/CGT/cgtnotes.pdf
.. [14] https://webusers.imj-prg.fr/~jean.michel/gap3/htm/chap007.htm#SECT046

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 17, 2019

Member

The link should preferably point to an official gap-system page.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@jksuom I've not included the maple reference because it describes specifically about Fp groups only.

elm = g**p
if not H.contains(elm):
for i in range(len(H)):
H = PermutationGroup([H[i], elm])

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 19, 2019

Member

It looks H will be repeatedly redefined (and a single H[i] included). Perhaps this should be something like

H = PermutationGroup(H.generators + [elm])

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 19, 2019

Author Member

yes, that's right.

G = H
gns = G.generators
f = 0
if Integer(r) != 1:

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 19, 2019

Member

Why not if r != 1?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 19, 2019

Author Member

Please look at the below comment and I think even in python 1.00 = 1 (r = G.order()/H.order() will result in 1.0) but comparing a float with int is a bad idea.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 19, 2019

Author Member

or may be we can replace r = G.order()/H.order() with r = G.order()//H.order()

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 19, 2019

Member

Yes, I think that integer division // should be used.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 19, 2019

Author Member

Ok!

gns = G.generators
f = 0
if Integer(r) != 1:
for k, v in factorint(Integer(r)).items():

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 19, 2019

Author Member

In this line we have to use Integer(r) may be in that flow I have changed above as well.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

@jksuom can this be merged.

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

I'll have look into the algorithm more carefully. There should be some kind of explanation.
There could also be more substantial examples, with prime power invariants.
I also think that the name could be abelian_invariants. It looks better although it is longer.

for p in primefactors(G.order()):
ranks = []
r = 2
while(r != 1):

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 21, 2019

Member
Suggested change
while(r != 1):
while r > 1:
gns = G.generators
f = 0
if r != 1:
for k, v in factorint(r).items():

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 21, 2019

Member

r is a power of p so there is only one item in factorint(r). multiplicity(p, r) is cheaper and would suffice.

f += v
ranks.append(f)

if len(ranks):

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 21, 2019

Member

if ranks: would probably suffice.

if len(ranks):
l = []
for i in range(ranks[0]):
l.append(1);

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 21, 2019

Member

Could these 3 lines be replaced by l = [1]*ranks[0]?

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Thanks, for the comments actually my university exams has started so I'll respond with the changes as soon as possible!

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@jksuom have a look!

H = der
for g in gns:
elm = g**p
if not H.contains(elm):

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 23, 2019

Member

contains is a fairly expensive method since a coset factorization has to be computed. I think that the test should be taken with the same (fixed) H for all powers g**p. That filtered list of powers could probably also used as gens for the next round of the r loop instead of G.generators that is actually H.generators and thus contains also generators of der.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

By fixed H you mean we should avoid H = der? and use the same H as computed in the previous iteration.

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 25, 2019

Member

I mean that H should not be redefined for each g in gns (whose power is not contained in it). Instead, we should collect those powers g**p that are not in H in a list and then update H by adding all those new generators at once instead of one at a time. (So H is fixed during one for loop.)

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

Okay, I got it!

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

Done!, please have a look.

H = PermutationGroup(H.generators + l)
r = G.order()//H.order()
G = H
gns = G.generators

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 25, 2019

Member

gns now contains also the generators of der. Would it suffice to take only those that are in the list l of powers?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

I've checked and yes it's consistent with the previous result.

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 25, 2019

Member

In that case, I would suggest that the creation of l above be changed. Maybe something like this could work:

pows = [g**p for g in gns]
gns = [pow in pows if not H.contains(pow)]

It should be possible to use the name gns (or maybe gens) at this point already.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

I think the above gns should be
gns = [pow for pow in pows if not H.contains(pow)]
and in that case does it really make a difference, for me both(current and the above mentioned changes) looks same.

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 25, 2019

Member

Yes, of course. I did forget to add the for part.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 25, 2019

Author Member

After applying above changes one of the test-case seems to fail i.e the result is not consistent with GAP.

>>> G = AbelianGroup(2,3,4)
>>> assert G.abelian_invariants() == [2, 3, 4]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AssertionError
>>> G.abelian_invariants()
[2, 4]

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 25, 2019

Member

Perhaps this H = PermutationGroup(H.generators + l) should have dir.generators instead of the changing H.generators.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Apr 26, 2019

Author Member

H = PermutationGroup(H.generators + l) works fine the above failure is encountered after below changes:

pows = [g**p for g in gns]
gns = [pow for pow in pows if not H.contains(pow)]

And the reason seems to be same that you have mentioned above after calculating pows we are updating H = PermutationGroup(H.generators + pows) where H.generators is always der.generators.

So, I think the current commit is okay!

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@jksuom any updates on this?

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

I have been intending to create some some more substantial examples and study the workings of the algorithm on those but have been otherwise occupied, sorry. Perhaps you could find some non-abelian examples with several prime power invariants.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Okay but, it may take some because currently my exams are going on so, I'll be able to push the examples in the next week!

elm = g**p
if not H.contains(elm):
l.append(elm)
H = PermutationGroup(H.generators + l)

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 30, 2019

Member

Minor optimization:

 H = PermutationGroup(H.generators + l) if l else H
r = 2
while r > 1:
H = der
l = []

This comment has been minimized.

Copy link
@jksuom

jksuom Apr 30, 2019

Member

How about using something more informative like pows instead of l (that may sometimes look like 1)?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 May 1, 2019

Author Member

That's right I myself got confused between 1 and l several times.

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Here is one simple but not quite trivial example.

In [24]: t = Permutation(0, 1)

In [25]: s = Permutation(0, 2, 4, 6)(1, 3, 5, 7)

In [26]: G = PermutationGroup([t, s])

In [32]: G.abelian_invariants()
Out[32]: [2, 4]

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Would it be possible to set H = G.derived_subgroup() in the beginning and then

K = PermutationGroup(H.generators + l) if l else H
r = G.order()//K.order()
G = K

That way, there would be no need to redefine H = der on each round of the loop.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Thanks, for the comments. I'll push the changes addressed above next week.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

@jksuom I've thought of some examples but these seems to be abelian.

G = PermutationGroup([Permutation(0, 27, 11, 13, 17, 21, 23, 29)(7, 11, 13, 17, 21, 23)])
assert G.abelian_invariants() == [9]

G = PermutationGroup([Permutation(1, 2),Permutation(0, 29, 11, 13, 17, 31)(27, 23, 17)])
assert G.abelian_invariants() == [2, 8]

Do you have any idea of other examples?

@jksuom

This comment has been minimized.

Copy link
Member

commented May 7, 2019

Some non-commutative p-groups can be easily constructed as Sylow subgroups. Of course, only one p is involved but that would suffice to run the main loop once.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@jksuom have a look! Does it requires more improvement.

@jksuom

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I think that the while loop would look better if there would be no need to set the artificial r = 2 in the beginning. Something like this, for instance:

        while True:
            H = self.derived_subgroup()
            pows = []
            for g in gns:
                elm = g**p
                if not H.contains(elm):
                    pows.append(elm)
            H = PermutationGroup(H.generators + pows) if pows else H
            r = G.order()//H.order()
            G = H
            gns = pows
            if r == 1:
                break
            ranks.append(multiplicity(p, r))

Then perhaps this comment could also be implemented. There could also be something like Hgens = H.generators in the beginning. A local Hgens is shorter (and slightly more easily available).

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

I don't think defining Hgens = H.generators in the beginning will help!
The current diff after above mentioned changes is-

         gns = self.generators
         inv = []
         G = self
+        H = G.derived_subgroup()
         for p in primefactors(G.order()):
             ranks = []
-            r = 2
-            while r > 1:
-                H = self.derived_subgroup()
+            while True:
                 pows = []
                 for g in gns:
                     elm = g**p
                     if not H.contains(elm):
                         pows.append(elm)
-                H = PermutationGroup(H.generators + pows) if pows else H
-                r = G.order()//H.order()
-                G = H
+                K = PermutationGroup(H.generators + pows) if pows else H
+                r = G.order()//K.order()
+                G = K
                 gns = pows
-                if r != 1:
-                    ranks.append(multiplicity(p, r))
+                if r == 1:
+                    break;
+                ranks.append(multiplicity(p, r))
 

And finally we are updating the H in while loop so again we have to use H.generators only!

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Or may be you are thinking about using Hgens = H.generators inside the while loop just above where H.generators is used for easy access?

@jksuom

This comment has been minimized.

Copy link
Member

commented May 10, 2019

we are updating the H in while loop

Is that necessary? It now has to be reset at the beginning of the loop.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

Sorry, perhaps I got confused with K and H!.

@jksuom

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Thanks, I think this is ready to be merged.

@jksuom jksuom merged commit e557371 into sympy:master May 13, 2019

3 checks passed

codecov/project 73.847% (target 0%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented May 13, 2019

Thanks!!

@divyanshu132 divyanshu132 deleted the divyanshu132:Abelian-invariants branch May 13, 2019

@divyanshu132 divyanshu132 added the GSoC label Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.