Added kth roots to Permutation #35053
Conversation
Added integer_partition_with_given_parts in partition.py (for use in kth roots computations).
Codecov ReportBase: 88.60% // Head: 88.59% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #35053 +/- ##
===========================================
- Coverage 88.60% 88.59% -0.02%
===========================================
Files 2136 2136
Lines 396141 396267 +126
===========================================
+ Hits 351009 351070 +61
- Misses 45132 45197 +65
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
|
Further minor things:
|
Was redundant. Added comas.
Indeed! Thanks. How did I miss that...
I agree (especially because .kth_roots() compute by default the square roots), but if someone wants to extend this to Weyl groups or Coxeter groups, "roots" would feel weird I think.
You're right. Even thought I think "number_of_kth_roots" is better fit in this module, with its sibblings. I've added the info in doc. (Changed the name of |
|
@fchapoton, did you see my comment? I think it is important, because this might create a huge mess in the near future. |
Excellent point. I still hope for a better name, but maybe there is none.
|
|
I feel we should call this There should not be a default for Remove also the change (removing a line) to Add tests for corner cases of Make sure pyflakes is run. I think Some ways you can get more performance:
This whole part seems to be creating a lot of unnecessary objects: I would refactor out the cardinality method into a standalone function. Likewise, you can just use the |
Change kth to nth Improve number_of_nth_roots (better way to iter and more readable) Idem for has_nth_root Minor improvements in nth_roots
src/sage/combinat/permutation.py
Outdated
|
|
||
| A k-th root of the permutation self is a permutation Gamma such that Gamma^k == self. | ||
| A n-th root of the permutation ``self`` is a permutation `\gamma` such that `\gamma^n == self`. |
There was a problem hiding this comment.
It is now An n-th root :-)
src/sage/combinat/permutation.py
Outdated
| INPUT: | ||
|
|
||
| - k -- optional integer (default 2), at least 1 | ||
| Note that the number of n-th roots only depend on the cyclic type of ``self``. |
There was a problem hiding this comment.
I think it is cycle type, rather than cyclic type.
There was a problem hiding this comment.
Indeed. I'm confused by the French notation.
src/sage/combinat/permutation.py
Outdated
| @@ -5322,7 +5325,7 @@ def kth_roots(self, k=2): | |||
|
|
|||
| def merging_cycles(list_of_cycles): | |||
| """ | |||
| Generate all l-cycles such that its k-th power is the product of cycles in Cycles (which conctains gcd(l, k) cycles of lenght l/gcd(l, k)) | |||
| Generate all l-cycles such that its n-th power is the product of cycles in Cycles (which conctains gcd(l, n) cycles of lenght l/gcd(l, n)) | |||
There was a problem hiding this comment.
This docstring is a bit unclear. In particular, what is Cycles?
src/sage/combinat/permutation.py
Outdated
| """ | ||
| M = [0 for _ in L] | ||
| m = len(M) | ||
| for j in range(m): | ||
| M[(j*k)%m] = L[j] | ||
| M[(j*n)%m] = L[j] |
There was a problem hiding this comment.
The linter will require this to be written as M[(j * k) % m].
There was a problem hiding this comment.
I don't think so. Plus PEP8 says we can put higher precedence operators without spaces (and [] counts as an operator). I don't think the linter is (nor should be) that strict.
src/sage/combinat/permutation.py
Outdated
| @@ -5362,18 +5365,19 @@ def rewind(L, k): | |||
| Cycles[lc] = [] | |||
| Cycles[lc].append(c) | |||
|
|
|||
| # for each length m, collects all product of cycles which k-th power gives the product prod(Cycles[l]) | |||
| # for each length m, collects all product of cycles which n-th power gives the product prod(Cycles[l]) | |||
| Possibilities = {m: [] for m in Cycles} | |||
There was a problem hiding this comment.
I don't think you need a dict here, since you prepare the entries for each cycle separately. Also, variable names are usually lowercase.
src/sage/combinat/permutation.py
Outdated
| for m, N in Cycles.items(): | ||
| N = Integer(N) # I don't know why but ._findfirst doesn't work without | ||
| parts = [x for x in divisors(n) if gcd(m*x, n) == x] | ||
| if not Partitions(0, parts_in=[])._findfirst(N, parts): |
There was a problem hiding this comment.
This is a really strange line of code. Partitions(0, parts_in=[]) is simply the empty partition. _findfirst is a method without documentation, what is it suppose to do?
There was a problem hiding this comment.
My goal here is just to know if there exists a partition of N with parts in parts. I haven't been able to find an efficient method to do it.
Here, I create a parent Partitions_parts_in (writing Partitions_parts_in(N, parts) does not seem to work), and then ask for the first element.
._findfirst is the method that the class Partitions_parts_in uses to iterate over the partitions, if i understand well.
If you have a better idea, I'll take it!
There was a problem hiding this comment.
Partitions(N, parts_in=parts).is_empty()
src/sage/combinat/permutation.py
Outdated
| P = Permutations(self.size()) | ||
| Cycles = self.cycle_type().to_exp_dict() | ||
| Result = 1 | ||
| for m, N in Cycles.items(): |
There was a problem hiding this comment.
it might be helpful to rename m to length and N to multiplicity, or perhaps m. Also, d might be a good name for a divisor, instead of x.
There was a problem hiding this comment.
N is not a multiplicity, it is the number of cycles of a given length.
I've changed the others.
There was a problem hiding this comment.
Yes, multiplicty of the cycle length
src/sage/combinat/permutation.py
Outdated
| Result = 1 | ||
| for m, N in Cycles.items(): | ||
| parts = [x for x in divisors(n) if gcd(m*x, n) == x] | ||
| Result *= sum(SetPartitions(N, pa).cardinality() * |
There was a problem hiding this comment.
It might be helpful to explain this computation in a comment.
There was a problem hiding this comment.
... and not use caml case for variable names, ie Cycles -> cycles, Result -> result. Caml case is reserved for class names.
|
I'd like to mention that I would first like to have this code be readable, and then check whether it performs well. |
|
ping? |
|
Courage, Germain ! Ca demande une bonne dose de ténacité, mais tu as la chance d'avoir trouvé un rapporteur. Si tu n'es pas disponible, tu peux lui dire quand tu le seras. |
|
@GermainPoullot ping? |
|
I have updated the branch by merging the latest beta. This is to allow the checks to be run again. |
remove some spaces in empty lines
|
I have once more updated the branch by merging the latest beta. This is to allow the checks to be run again. |
variables en minuscules
some details
less whitespace
|
I am afraid I have broken some things when doing my changes.. |
some fixes
|
I have fixed things and doctests now pass. |
less whitespace
detail
detail
suggested details
|
Thank you for doing this! I think this is ready to go, as soon as the bot has finished! |
|
just a typo: |
fix doctest
src/sage/combinat/permutation.py
Outdated
| r""" | ||
| Decide if ``self`` has n-th roots. | ||
|
|
||
| An n-th root of the permutation ``self`` is a permutation `\gamma` such that `\gamma^n == self`. |
There was a problem hiding this comment.
\gamma^n == self doesn't come out properly in the documentation. Not sure what it should be, possibly
``gamma`` such that ``gamma^n == self``
or
of the permutation `\sigma` is a permutation `\gamma` such that `\gamma^n = \sigma`.
I don't know how to do it properly.
The same comment applies to the docstrings of n_th_roots and number_of_nth_roots.
I'm sorry :-(
There was a problem hiding this comment.
of the permutation `\sigma` is a permutation `\gamma` such that `\gamma^n = \sigma`.
This is the one I would do.
fixes in doc
|
LGTM |
|
Documentation preview for this PR (built with commit 13b52c5; changes) is ready! 🎉 |
### 📚 Description Added 3 functions that deal with k-th roots of permutations: 1) Permutation.kth_roots(k) compute a iterator of over all k-th roots of self. 2) Permutation.has_kth_root(k) determines if self has a k-th root (or several). 3) Permutation.number_of_kth_roots(k) returns the number of k-th roots of self. Added integer_partition_with_given_parts(n, parts) in partition.py (for use in k-th roots computations): it creates a iterator over all partitions of n which parts are in the variable parts. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If it resolves an open issue, please link to the issue here. For example "Closes sagemath#1337" --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [ ] I have linked an issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open pull requests that this PR logically depends on --> <!-- - #xyz: short description why this is a dependency - #abc: ... --> URL: sagemath#35053 Reported by: GermainPoullot Reviewer(s): Frédéric Chapoton, GermainPoullot, Martin Rubey, Travis Scrimshaw, Vincent Delecroix
### 📚 Description Added 3 functions that deal with k-th roots of permutations: 1) Permutation.kth_roots(k) compute a iterator of over all k-th roots of self. 2) Permutation.has_kth_root(k) determines if self has a k-th root (or several). 3) Permutation.number_of_kth_roots(k) returns the number of k-th roots of self. Added integer_partition_with_given_parts(n, parts) in partition.py (for use in k-th roots computations): it creates a iterator over all partitions of n which parts are in the variable parts. <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If it resolves an open issue, please link to the issue here. For example "Closes sagemath#1337" --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] I have made sure that the title is self-explanatory and the description concisely explains the PR. - [ ] I have linked an issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open pull requests that this PR logically depends on --> <!-- - #xyz: short description why this is a dependency - #abc: ... --> URL: sagemath#35053 Reported by: GermainPoullot Reviewer(s): Frédéric Chapoton, GermainPoullot, Martin Rubey, Travis Scrimshaw, Vincent Delecroix
📚 Description
Added 3 functions that deal with k-th roots of permutations:
Added integer_partition_with_given_parts(n, parts) in partition.py (for use in k-th roots computations):
it creates a iterator over all partitions of n which parts are in the variable parts.
📝 Checklist
⌛ Dependencies