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

Improve is_cyclic #17781

Merged
merged 9 commits into from
Oct 25, 2019
Merged

Improve is_cyclic #17781

merged 9 commits into from
Oct 25, 2019

Conversation

sylee957
Copy link
Member

References to other Issues or PRs

Brief description of what is fixed or changed

  1. There were some wrong results for is_cyclic
>>> PermutationGroup(
...     Permutation(0, 1, 2, 3),
...     Permutation(0, 2)(1, 3)
... ).is_cyclic
False

because of if not self._is_abelian: which gives a false positive for None

  1. I've made is_abelian cache to update whenever is_cyclic holds True.

  2. I've also added an additional lemma when the order is square-free.

Other comments

I've also updated the documentation
image

Release Notes

  • combinatorics
    • Fixed PermutationGroup.is_cyclic giving wrong result for the cyclic groups created with explicitly specified generators.
    • PermutationGroup.is_cyclic automatically updates the cache for is_abelian.
    • Improved the algorithm of PermutationGroup.is_cyclic to detect some trivial cases like the group order of 15, 35, etc.

@sympy-bot
Copy link

sympy-bot commented Oct 23, 2019

Hi, I am the SymPy bot (v149). 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
    • Fixed PermutationGroup.is_cyclic giving wrong result for the cyclic groups created with explicitly specified generators. (#17781 by @sylee957)

    • PermutationGroup.is_cyclic automatically updates the cache for is_abelian. (#17781 by @sylee957)

    • Improved the algorithm of PermutationGroup.is_cyclic to detect some trivial cases like the group order of 15, 35, etc. (#17781 by @sylee957)

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. -->

#### References to other Issues or PRs
<!-- 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

1. There were some wrong results for `is_cyclic`
```python3
>>> PermutationGroup(
...     Permutation(0, 1, 2, 3),
...     Permutation(0, 2)(1, 3)
... ).is_cyclic
False
```
because of `if not self._is_abelian:` which gives a false positive for `None`

2. I've made `is_abelian` cache to update whenever `is_cyclic` holds ``True``. 

3. I've also added an additional lemma when the order is square-free.

#### Other comments

I've also updated the documentation
![image](https://user-images.githubusercontent.com/34944973/67360735-9fbcc880-f5a1-11e9-9941-9c47d2ae8468.png)

#### 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
  - Fixed `PermutationGroup.is_cyclic` giving wrong result for the cyclic groups created with explicitly specified generators.
  - `PermutationGroup.is_cyclic` automatically updates the cache for `is_abelian`.
  - Improved the algorithm of `PermutationGroup.is_cyclic` to detect some trivial cases like the group order of `15`, `35`, etc.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@jksuom
Copy link
Member

jksuom commented Oct 23, 2019

Would it be expensive to check first that the group is abelian? Then, I think, _distinct_primes_lemma would not be needed. It would suffice that the exponents are 1.

@sylee957
Copy link
Member Author

is_abelian can take a lot of time if there are many generators.
I think that it even takes more time to test is_abelian first than using the default check when _distinct_primes_lemma fails, like

G = AbelianGroup(7, 29)
G = PermutationGroup(*G.generate())
G._is_abelian = None
G._is_cyclic = None

This case takes 236888 function calls in 0.100 seconds for is_cyclic, but takes 791745 function calls in 0.211 seconds for is_abelian.

I just added the suggestion as a trivial case when the cache is warm.

@jksuom
Copy link
Member

jksuom commented Oct 23, 2019

Then this looks like a good method. Still, I wonder why checking commutativity is so expensive. Maybe that could be improved.

@jksuom jksuom merged commit d85c30e into sympy:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants