Steinhaus-Johnson-Trotter algorithm for permutations #38064
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Documentation preview for this PR (built with commit f9d76cf; changes) is ready! 🎉 |
|
Linter is failing. I think a few of the other CI fails are fixed in some PRs, so maybe a rebase to trigger CI will resolve them. |
grhkm21
suggested changes
May 27, 2024
There was a problem hiding this comment.
You are also missing examples/doctests in docstrings everywhere - see developer guide, in particular the EXAMPLES block.
JosePisco
force-pushed
the
sjt-algorithm-for-permutations
branch
from
fe085c6
to
80ca849
Compare
grhkm21
suggested changes
May 30, 2024
|
Also can you resolve the comments that are fixed, so i can see what to look |
|
Many thanks again @grhkm21 for reviewing the code. I've made |
grhkm21
suggested changes
Jun 1, 2024
|
Ah thank you for catching these small style issues. Commit 1052244 meets the changes. |
Some doctring formatting has also been done.
JosePisco
force-pushed
the
sjt-algorithm-for-permutations
branch
from
1052244
to
6ed2a79
Compare
|
Last linting error has been fixed. If everybody agrees with the changes and the overall code suggestion, this PR is ready for merging on my side. |
|
Sorry for the delay |
grhkm21
approved these changes
Jun 8, 2024
| else: | ||
| self._directions = directions | ||
|
|
||
| def __idx_largest_element_non_zero_direction(self, perm, directions): |
There was a problem hiding this comment.
Usually internal methods are single underscores, but this is okay too
grhkm21
suggested changes
Jun 8, 2024
| if self._algorithm != "lex" and self._algorithm != "sjt": | ||
| raise ValueError("unsupported algorithm %s; expected 'lex' or 'sjt'" | ||
| % self._algorithm) | ||
|
|
There was a problem hiding this comment.
Suggested change
Sorry, please fix this (CI lint is failing)
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 10, 2024
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> I suggest this diff against develop to implement the Steinhaus-Johnson- Trotter algorithm that generates the permutations of a list using only transpositions of two elements of the list. The algorithm can be selected upon initialization with the parameter of the same name, defaults to "lex" which is the current algorithm. Since the `Permutation` class is a bit weird and creates a new list and a new object on every iteration, I had to tweak a bit the parameters in input to the class `__init__` method adding - `algorithm`: this one is fine, it allows to choose which algorithm to use to generate the permutations - `directions`: this one is meant to keep track of the internal state specific to the SJT algorithm. To find the two elements to transpose for the next permutation, we need to find elements according to some conditions regarding each of their direction. Since the `Permutation` class creates a new object at each iteration, I tried to find the best way to pass on the internal state to the new object. Maybe the internal state can be directly computed from the list, I haven't checked. Because of this dependency on the internal state, only the identity permutation can be used when initializing a `Permutation` object with `algorithm='sjt'` (see examples from doc). I am aware that adding an extra optional parameter to the object construction might not be the best option thus I will gladly take your feedback and opinions. I haven't implemented the `prev()` method. I think it should be done if this is accepted. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38064 Reported by: grnx Reviewer(s): grhkm21, grnx
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 16, 2024
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> I suggest this diff against develop to implement the Steinhaus-Johnson- Trotter algorithm that generates the permutations of a list using only transpositions of two elements of the list. The algorithm can be selected upon initialization with the parameter of the same name, defaults to "lex" which is the current algorithm. Since the `Permutation` class is a bit weird and creates a new list and a new object on every iteration, I had to tweak a bit the parameters in input to the class `__init__` method adding - `algorithm`: this one is fine, it allows to choose which algorithm to use to generate the permutations - `directions`: this one is meant to keep track of the internal state specific to the SJT algorithm. To find the two elements to transpose for the next permutation, we need to find elements according to some conditions regarding each of their direction. Since the `Permutation` class creates a new object at each iteration, I tried to find the best way to pass on the internal state to the new object. Maybe the internal state can be directly computed from the list, I haven't checked. Because of this dependency on the internal state, only the identity permutation can be used when initializing a `Permutation` object with `algorithm='sjt'` (see examples from doc). I am aware that adding an extra optional parameter to the object construction might not be the best option thus I will gladly take your feedback and opinions. I haven't implemented the `prev()` method. I think it should be done if this is accepted. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38064 Reported by: grnx Reviewer(s): grhkm21, grnx
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Jun 16, 2024
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> I suggest this diff against develop to implement the Steinhaus-Johnson- Trotter algorithm that generates the permutations of a list using only transpositions of two elements of the list. The algorithm can be selected upon initialization with the parameter of the same name, defaults to "lex" which is the current algorithm. Since the `Permutation` class is a bit weird and creates a new list and a new object on every iteration, I had to tweak a bit the parameters in input to the class `__init__` method adding - `algorithm`: this one is fine, it allows to choose which algorithm to use to generate the permutations - `directions`: this one is meant to keep track of the internal state specific to the SJT algorithm. To find the two elements to transpose for the next permutation, we need to find elements according to some conditions regarding each of their direction. Since the `Permutation` class creates a new object at each iteration, I tried to find the best way to pass on the internal state to the new object. Maybe the internal state can be directly computed from the list, I haven't checked. Because of this dependency on the internal state, only the identity permutation can be used when initializing a `Permutation` object with `algorithm='sjt'` (see examples from doc). I am aware that adding an extra optional parameter to the object construction might not be the best option thus I will gladly take your feedback and opinions. I haven't implemented the `prev()` method. I think it should be done if this is accepted. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38064 Reported by: grnx Reviewer(s): grhkm21, grnx
1 task
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I suggest this diff against develop to implement the Steinhaus-Johnson-Trotter algorithm that generates the permutations of a list using only transpositions of two elements of the list. The algorithm can be selected upon initialization with the parameter of the same name, defaults to "lex" which is the current algorithm.
Since the
Permutationclass is a bit weird and creates a new list and a new object on every iteration, I had to tweak a bit the parameters in input to the class__init__method addingalgorithm: this one is fine, it allows to choose which algorithm to use to generate the permutationsdirections: this one is meant to keep track of the internal state specific to the SJT algorithm. To find the two elements to transpose for the next permutation, we need to find elements according to some conditions regarding each of their direction. Since thePermutationclass creates a new object at each iteration, I tried to find the best way to pass on the internal state to the new object. Maybe the internal state can be directly computed from the list, I haven't checked. Because of this dependency on the internal state, only the identity permutation can be used when initializing aPermutationobject withalgorithm='sjt'(see examples from doc). I am aware that adding an extra optional parameter to the object construction might not be the best option thus I will gladly take your feedback and opinions.I haven't implemented the
prev()method. I think it should be done if this is accepted.📝 Checklist
⌛ Dependencies