Added method for Composition Series computation #16881

Merged
merged 11 commits into from Jun 4, 2019
+108 −2

Conversation

Projects
None yet
5 participants
Member

divyanshu132 commented May 23, 2019 • edited

Brief description of what is fixed or changed

Computation of composition-series has been implemented for finite solvable groups which should further help in computing with polycyclic group

1. Improve docs and tests.
GSoC

Release Notes

• combinatorics
• added method for Composition-Series computation
``` added-method-for-composition-series ```
``` 23480f8 ```

sympy-bot commented May 23, 2019 • edited

 ✅ 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: combinatorics added method for Composition-Series computation (#16881 by @divyanshu132) 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. `````` #### Brief description of what is fixed or changed Computation of composition-series has been implemented for finite solvable groups which should further help in computing with polycyclic group #### Other comments 1. Improve docs and tests. GSoC #### Release Notes * combinatorics * added method for Composition-Series computation `````` Update The release notes on the wiki have been updated.

divyanshu132 reviewed May 23, 2019

 break elm = [elm] for pelm in elm:

divyanshu132 May 23, 2019

Author Member

If the elm is in the form of `Permutation(1,2)(2,3)`, how can I extract both the elements in such a way so that further I can compute their order??

jksuom May 24, 2019

Member

This permutation is the cyclic permutation (1 3 2). It is the product of (1 2) and (2 3) but it can be represented as a product in many other ways.

Member Author

Codecov Report

Merging #16881 into master will increase coverage by `0.083%`.
The diff coverage is `75.862%`.

```@@              Coverage Diff              @@
##            master    #16881       +/-   ##
=============================================
+ Coverage   73.875%   73.959%   +0.083%
=============================================
Files          619       620        +1
Lines       159897    160393      +496
Branches     37542     37636       +94
=============================================
+ Hits        118125    118626      +501
+ Misses       36298     36282       -16
- Partials      5474      5485       +11```

jksuom reviewed May 24, 2019

 gns = low.generators while low != der[i]: while True: elm = der[i].random()

jksuom May 24, 2019

Member

I think that it would suffice to take the generators of der[i] instead of random elements.

Member Author

divyanshu132 commented May 24, 2019

 @oscarbenjamin please replace the `series` label with `combinatorics`.

divyanshu132 added some commits May 24, 2019

``` random-to-generators ```
``` 47f31a8 ```
``` add-docs-tests ```
``` 919bcbd ```

jksuom reviewed May 25, 2019

 for pelm in elm: order = pelm.order() div = divisors(order)[1] l = Integer(log(order, div))

jksuom May 25, 2019

Member

This could be `multiplicity(div, order)`. Or `factorint(order)` would give both the prime factors and their multiplicity.

jksuom reviewed May 25, 2019

sympy/combinatorics/perm_groups.py Outdated
``` update ```
``` 36ee514 ```

czgdp1807 reviewed May 26, 2019

 low = der[i+1] gns = low.generators for elm in der[i].generators: if low == der[i]:

czgdp1807 May 26, 2019

Member

Is it guaranteed that `low` and `der[i]` will always be `number` and not expressions containing `Symbol`?

divyanshu132 May 26, 2019

Author Member

They are neither numbers nor expressions, they are perm groups.

``` test-cyclicgroup ```
``` 9a696fb ```
Member Author

divyanshu132 commented May 27, 2019

``` minor-changes ```
``` 16cad25 ```

jksuom reviewed May 27, 2019

 H = der[i+1] Hgens = H.generators for g in der[i].generators: order = g.order()

jksuom May 27, 2019

Member

This should be the relative order `K.order() // H.order()`.

jksuom reviewed May 27, 2019

 Hgens = H.generators for g in der[i].generators: order = g.order() series_segment = []

jksuom May 27, 2019

Member

This should be outside of g loop.

``` use-relative-order-seg ```
``` 6377973 ```
Member

jksuom commented May 29, 2019

 There is something wrong: ``````In [1]: from sympy.combinatorics import * In [2]: S = SymmetricGroup(8) In [3]: G = S.sylow_subgroup(2) In [4]: C = G.composition_series() In [5]: len(C) Out[5]: 5 In [6]: G.order() Out[6]: 128 In [7]: [H.order() for H in C] Out[7]: [1, 2, 4, 32, 128] `````` There should be one group for each power of 2 between 1 and 128. It is also expected that the groups are in descending order: `G = H_0 > H_1 > H_2 >`.

divyanshu132 added some commits May 30, 2019

``` dummy-implementation ```
``` e96661c ```
``` update ```
``` 454ddc4 ```
Member Author

divyanshu132 commented May 31, 2019

 @jksuom have a look, I'll include the algorithm by tomorrow. I've tested on some cases mentioned in the docstring please have a look!

jksuom reviewed May 31, 2019

 g = g**p else: g = g**(p**j) if not g in H:

jksuom May 31, 2019

Member

It should not be necessary to check `g in H`. That is relatively expensive and can be avoided if `order` is the least integer such that `g**order` is in `H`.

jksuom reviewed May 31, 2019

 order = g.order() for p, e in factorint(order).items(): for j in range(e): if len(der) == 2:

jksuom May 31, 2019

Member

The algorithm should handle `der` of all sizes in a uniform way. An extra check in the innermost loop is a waste of time.

divyanshu132 May 31, 2019

Author Member

Okay, I'll try removing it!

``` use-relative-order ```
``` 5f554d9 ```
Member Author

divyanshu132 commented Jun 2, 2019

 @jksuom have a look.

jksuom reviewed Jun 3, 2019

 """ if not self.is_solvable: raise NotImplementedError('Group should be solvable') der = self.derived_series()

jksuom Jun 3, 2019

Member

The derived series is also computed above in `self.is_solvable`. It would be possible to avoid repetition if this series is computed first and then checked that the last group is trivial (`all(g.is_identity for g in der[-1].generators)`).

jksuom reviewed Jun 3, 2019

 g = g**p up_seg = down_seg + up_seg H = K series.extend(up_seg)

jksuom Jun 3, 2019

Member

`up_seg[0]` is `der[i]` with extra generators coming from `der[i+1]`. The result could be simplified by omitting the extra generators: `up_seg[0] = der[i]` before extending `series`.

jksuom reviewed Jun 3, 2019

 up_seg = down_seg + up_seg H = K series.extend(up_seg) series.append(PermutationGroup([self.identity()]))

jksuom Jun 3, 2019

Member

This could probably be `series.append(der[-1])`.

divyanshu132 Jun 4, 2019

Author Member

Done!

``` avoid-der-repetition ```
``` ac4dc7c ```
Member

jksuom commented Jun 4, 2019

 I think this is ready to be merged. Thanks!

jksuom merged commit `e827e19` into sympy:master Jun 4, 2019 3 checks passed

3 checks passed

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

divyanshu132 commented Jun 4, 2019

 Thanks, you helped a lot!