-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 exponent for permutation group #18888
base: master
Are you sure you want to change the base?
Conversation
✅ Hi, I am the SymPy bot (v158). 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.6. 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.
|
Please review @jksuom |
Codecov Report
@@ Coverage Diff @@
## master #18888 +/- ##
=============================================
- Coverage 75.668% 75.653% -0.015%
=============================================
Files 647 647
Lines 168520 168524 +4
Branches 39709 39710 +1
=============================================
- Hits 127516 127495 -21
- Misses 35448 35471 +23
- Partials 5556 5558 +2 |
""" | ||
Returns the exponent of a group | ||
|
||
The exponent e of a group G is the lcm of the orders of its elements, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an existing method in sympy to compute lcm.
exp = 1
for g in self.generators:
exp = lcm(exp, g.order())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you dedent the docstrings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure do we need to do it for docstring too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s only for the code convention
assert A.exponent() == 12 | ||
|
||
c = Permutation(1,2,3,4,5,6)(2,3,4)(5,6,7)(8,9)(10,11,12) | ||
A = PermutationGroup([a,b,c]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should add spacing around commas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should have [a, b, c]
but I think that the previous line can be left as it is. Otherwise it will grow in size unreasonably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I have changed it.
The orders of all elements are needed for the exponent, generators alone do not suffice.
|
Yes right I have done the changes. |
assert A.exponent() == 12 | ||
|
||
c = Permutation(1,2,3,4,5,6)(2,3,4)(5,6,7)(8,9)(10,11,12) | ||
A = PermutationGroup([a, b, c]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change this too its order is to large to be used for test?
>>> A.order()
15120
why is it failing? it is passing on my system. |
I think that this is not a practical method. It should not be necessary to compute all elements and their orders. What does GAP do? |
they calculate the something like:
|
That seems a reasonable method. The exponent of a finite group is the product of the exponents of its Sylow subgroups. |
In case when
|
If that is the case perhaps, you can have a set having all the sylow-p subgroups to terminate the loop you can probably use break statement if the resultant subgroup is already there in the set. |
I have done the changes. Please look will they suffice. |
Please review the changes. |
Does GAP use a special method for computing the exponent of a p-group? |
Ahh... I did't find any they have a |
Maybe for solvable groups. p-groups a solvable, even nilpotent. |
sry I again did'nt find any :( I searched extensively |
maybe It can be improved more by applying the following property:
|
Hello @jksuom , After edit : - |
if sylow_p_subgroup not in sylow_groups: | ||
elements = list(sylow_p_subgroup.elements) | ||
exp_p = elements[0].order() | ||
for i in range(1, len(elements)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks inefficient. len(elements)
may be very big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, got it
I looked at the gap implementation that method will require the computation of many other methods too such Leedham-Green series
and special pcgs
and many others too.
should I go ahead and implement those in new pr's and after that jump to this pr again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that something like that will be needed. If there are no algorithms in the Handbook, then those method may be hard to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right It can be hard probably I can take help of gap implementation (as of now they look possible to implement to me but there need to be too many implemented) but implementing these may take too much time so first I will try to open a pr on which I have already worked and they are on my system.
@mohitacecode this has merge conflicts now |
@mohitacecode Please resolve the conflicts. Thanks for your contributions. |
References to other Issues or PRs
Brief description of what is fixed or changed
A method is added to calculate the exponent of a permutation group
Other comments
Release Notes
exponent
inperm_groups.py
.