-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Method braid of class Link loops when the Link contains loops #36884
Conversation
None of the failures from the bot-logfile appear to be related to this PR. |
Co-authored-by: Travis Scrimshaw <[email protected]>
Co-authored-by: Travis Scrimshaw <[email protected]>
Co-authored-by: Travis Scrimshaw <[email protected]>
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.
Thank you.
Just one small additional change; once done, you can set a positive review.
Co-authored-by: Travis Scrimshaw <[email protected]>
Thanks! |
Documentation preview for this PR (built with commit f68c72a; changes) is ready! 🎉 |
sagemathgh-36884: Method braid of class Link loops when the Link contains loops <!-- ^^^^^ 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 this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> The following link `L` is ambient isotopic to the treefoil knot but has one additional loop.  ``` sage: L = Link([[1, 7, 2, 6], [3, 1, 4, 8], [5, 5, 6, 4], [7, 3, 8, 2]]) sage: L.regions()` [[-5], [8, 4, 6, 2], [7, -2], [3, -8], [1, -6, 5, -4], [-1, -3, -7]] sage: L.seifert_circles() [[5], [1, 7, 3], [2, 8, 4, 6]] ``` Invoking `L.braid()` does not terminate. The problem already occurs on the first application of a Vogel move which leads to an extended `pd_code = [[10, 7, 2, 6], [3, 1, 4, 8], [11, 5, 6, 4], [7, 3, 8, 2], [12, 9, 5, 1], [11, 9, 12, 10]]`. A diagram of the corresponding link looks like this:  The labels `C1`, `C2 = [3, 1, 4, 8]`, `D = [11, 9, 12, 10]` and `E = [12, 9, 5, 1]` correspond to the variables in the code and the numbers to the edges. In case of `C1`the correct list of edges would be `[5, 11, 6, 4]` but the code produces `[11, 5, 6, 4]`. This is caused by usage of the `index` method of the `list` class which always returns the first occurrence of a value in the list, i.e. by the line: ``` C2[C2.index(b)] = newedge + 2 ``` To fix it I replace the method by a local function: ``` def idx(cross, edge): r""" Return the index of an edge in a crossing taking loops into account. A loop appears as an edge which occurs twice in the crossing. In all cases the second occurrence is the correct one needed in the Vogel algorithm. """ i = cross.index(edge) if cross.count(edge) > 1: return cross.index(edge, i+1) else: return i ``` > In all cases the second occurrence is the correct one needed in the Vogel algorithm. The according argumentation goes as follows: If the loop is clockwise oriented the edge has a positive sign in the region, else a negative sign. In the first case the outgoing side of the edge always occurs after the incoming one in the PD-code (edge 1 in the first picture below and edge 2 in the second). In the second case the incoming side of the edge always occurs after the outgoing one in the PD-code (edge 2 in the first picture below and edge 1 in the second).   Thus, this corresponds to the fact that in the first case `a` and `b` leave the crossings `C1` and `C2` (in the original link) whereas they arrive there in the second case. Another way to fix the issue would be to remove all loops from the diagram before starting the Vogel algorithm. But this would harm regular isotopy which might violate the expectation of the user. Thus, I implement this as an optional feature. This gives the user the possibility to obtain a braid of smaller index if he is fine with ambient isotopy. ### 📝 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! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36884 Reported by: Sebastian Oehms Reviewer(s): Sebastian Oehms, Travis Scrimshaw
sagemathgh-36884: Method braid of class Link loops when the Link contains loops <!-- ^^^^^ 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 this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> The following link `L` is ambient isotopic to the treefoil knot but has one additional loop.  ``` sage: L = Link([[1, 7, 2, 6], [3, 1, 4, 8], [5, 5, 6, 4], [7, 3, 8, 2]]) sage: L.regions()` [[-5], [8, 4, 6, 2], [7, -2], [3, -8], [1, -6, 5, -4], [-1, -3, -7]] sage: L.seifert_circles() [[5], [1, 7, 3], [2, 8, 4, 6]] ``` Invoking `L.braid()` does not terminate. The problem already occurs on the first application of a Vogel move which leads to an extended `pd_code = [[10, 7, 2, 6], [3, 1, 4, 8], [11, 5, 6, 4], [7, 3, 8, 2], [12, 9, 5, 1], [11, 9, 12, 10]]`. A diagram of the corresponding link looks like this:  The labels `C1`, `C2 = [3, 1, 4, 8]`, `D = [11, 9, 12, 10]` and `E = [12, 9, 5, 1]` correspond to the variables in the code and the numbers to the edges. In case of `C1`the correct list of edges would be `[5, 11, 6, 4]` but the code produces `[11, 5, 6, 4]`. This is caused by usage of the `index` method of the `list` class which always returns the first occurrence of a value in the list, i.e. by the line: ``` C2[C2.index(b)] = newedge + 2 ``` To fix it I replace the method by a local function: ``` def idx(cross, edge): r""" Return the index of an edge in a crossing taking loops into account. A loop appears as an edge which occurs twice in the crossing. In all cases the second occurrence is the correct one needed in the Vogel algorithm. """ i = cross.index(edge) if cross.count(edge) > 1: return cross.index(edge, i+1) else: return i ``` > In all cases the second occurrence is the correct one needed in the Vogel algorithm. The according argumentation goes as follows: If the loop is clockwise oriented the edge has a positive sign in the region, else a negative sign. In the first case the outgoing side of the edge always occurs after the incoming one in the PD-code (edge 1 in the first picture below and edge 2 in the second). In the second case the incoming side of the edge always occurs after the outgoing one in the PD-code (edge 2 in the first picture below and edge 1 in the second).   Thus, this corresponds to the fact that in the first case `a` and `b` leave the crossings `C1` and `C2` (in the original link) whereas they arrive there in the second case. Another way to fix the issue would be to remove all loops from the diagram before starting the Vogel algorithm. But this would harm regular isotopy which might violate the expectation of the user. Thus, I implement this as an optional feature. This gives the user the possibility to obtain a braid of smaller index if he is fine with ambient isotopy. ### 📝 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! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36884 Reported by: Sebastian Oehms Reviewer(s): Sebastian Oehms, Travis Scrimshaw
sagemathgh-36884: Method braid of class Link loops when the Link contains loops <!-- ^^^^^ 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 this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> The following link `L` is ambient isotopic to the treefoil knot but has one additional loop.  ``` sage: L = Link([[1, 7, 2, 6], [3, 1, 4, 8], [5, 5, 6, 4], [7, 3, 8, 2]]) sage: L.regions()` [[-5], [8, 4, 6, 2], [7, -2], [3, -8], [1, -6, 5, -4], [-1, -3, -7]] sage: L.seifert_circles() [[5], [1, 7, 3], [2, 8, 4, 6]] ``` Invoking `L.braid()` does not terminate. The problem already occurs on the first application of a Vogel move which leads to an extended `pd_code = [[10, 7, 2, 6], [3, 1, 4, 8], [11, 5, 6, 4], [7, 3, 8, 2], [12, 9, 5, 1], [11, 9, 12, 10]]`. A diagram of the corresponding link looks like this:  The labels `C1`, `C2 = [3, 1, 4, 8]`, `D = [11, 9, 12, 10]` and `E = [12, 9, 5, 1]` correspond to the variables in the code and the numbers to the edges. In case of `C1`the correct list of edges would be `[5, 11, 6, 4]` but the code produces `[11, 5, 6, 4]`. This is caused by usage of the `index` method of the `list` class which always returns the first occurrence of a value in the list, i.e. by the line: ``` C2[C2.index(b)] = newedge + 2 ``` To fix it I replace the method by a local function: ``` def idx(cross, edge): r""" Return the index of an edge in a crossing taking loops into account. A loop appears as an edge which occurs twice in the crossing. In all cases the second occurrence is the correct one needed in the Vogel algorithm. """ i = cross.index(edge) if cross.count(edge) > 1: return cross.index(edge, i+1) else: return i ``` > In all cases the second occurrence is the correct one needed in the Vogel algorithm. The according argumentation goes as follows: If the loop is clockwise oriented the edge has a positive sign in the region, else a negative sign. In the first case the outgoing side of the edge always occurs after the incoming one in the PD-code (edge 1 in the first picture below and edge 2 in the second). In the second case the incoming side of the edge always occurs after the outgoing one in the PD-code (edge 2 in the first picture below and edge 1 in the second).   Thus, this corresponds to the fact that in the first case `a` and `b` leave the crossings `C1` and `C2` (in the original link) whereas they arrive there in the second case. Another way to fix the issue would be to remove all loops from the diagram before starting the Vogel algorithm. But this would harm regular isotopy which might violate the expectation of the user. Thus, I implement this as an optional feature. This gives the user the possibility to obtain a braid of smaller index if he is fine with ambient isotopy. ### 📝 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! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36884 Reported by: Sebastian Oehms Reviewer(s): Sebastian Oehms, Travis Scrimshaw
sagemathgh-36884: Method braid of class Link loops when the Link contains loops <!-- ^^^^^ 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 this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> The following link `L` is ambient isotopic to the treefoil knot but has one additional loop.  ``` sage: L = Link([[1, 7, 2, 6], [3, 1, 4, 8], [5, 5, 6, 4], [7, 3, 8, 2]]) sage: L.regions()` [[-5], [8, 4, 6, 2], [7, -2], [3, -8], [1, -6, 5, -4], [-1, -3, -7]] sage: L.seifert_circles() [[5], [1, 7, 3], [2, 8, 4, 6]] ``` Invoking `L.braid()` does not terminate. The problem already occurs on the first application of a Vogel move which leads to an extended `pd_code = [[10, 7, 2, 6], [3, 1, 4, 8], [11, 5, 6, 4], [7, 3, 8, 2], [12, 9, 5, 1], [11, 9, 12, 10]]`. A diagram of the corresponding link looks like this:  The labels `C1`, `C2 = [3, 1, 4, 8]`, `D = [11, 9, 12, 10]` and `E = [12, 9, 5, 1]` correspond to the variables in the code and the numbers to the edges. In case of `C1`the correct list of edges would be `[5, 11, 6, 4]` but the code produces `[11, 5, 6, 4]`. This is caused by usage of the `index` method of the `list` class which always returns the first occurrence of a value in the list, i.e. by the line: ``` C2[C2.index(b)] = newedge + 2 ``` To fix it I replace the method by a local function: ``` def idx(cross, edge): r""" Return the index of an edge in a crossing taking loops into account. A loop appears as an edge which occurs twice in the crossing. In all cases the second occurrence is the correct one needed in the Vogel algorithm. """ i = cross.index(edge) if cross.count(edge) > 1: return cross.index(edge, i+1) else: return i ``` > In all cases the second occurrence is the correct one needed in the Vogel algorithm. The according argumentation goes as follows: If the loop is clockwise oriented the edge has a positive sign in the region, else a negative sign. In the first case the outgoing side of the edge always occurs after the incoming one in the PD-code (edge 1 in the first picture below and edge 2 in the second). In the second case the incoming side of the edge always occurs after the outgoing one in the PD-code (edge 2 in the first picture below and edge 1 in the second).   Thus, this corresponds to the fact that in the first case `a` and `b` leave the crossings `C1` and `C2` (in the original link) whereas they arrive there in the second case. Another way to fix the issue would be to remove all loops from the diagram before starting the Vogel algorithm. But this would harm regular isotopy which might violate the expectation of the user. Thus, I implement this as an optional feature. This gives the user the possibility to obtain a braid of smaller index if he is fine with ambient isotopy. ### 📝 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! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36884 Reported by: Sebastian Oehms Reviewer(s): Sebastian Oehms, Travis Scrimshaw
The following link
L
is ambient isotopic to the treefoil knot but has one additional loop.Invoking
L.braid()
does not terminate. The problem already occurs on the first application of a Vogel move which leads to an extendedpd_code = [[10, 7, 2, 6], [3, 1, 4, 8], [11, 5, 6, 4], [7, 3, 8, 2], [12, 9, 5, 1], [11, 9, 12, 10]]
. A diagram of the corresponding link looks like this:The labels
C1
,C2 = [3, 1, 4, 8]
,D = [11, 9, 12, 10]
andE = [12, 9, 5, 1]
correspond to the variables in the code and the numbers to the edges. In case ofC1
the correct list of edges would be[5, 11, 6, 4]
but the code produces[11, 5, 6, 4]
. This is caused by usage of theindex
method of thelist
class which always returns the first occurrence of a value in the list, i.e. by the line:To fix it I replace the method by a local function:
The according argumentation goes as follows:
If the loop is clockwise oriented the edge has a positive sign in the region, else a negative sign. In the first case the outgoing side of the edge always occurs after the incoming one in the PD-code (edge 1 in the first picture below and edge 2 in the second). In the second case the incoming side of the edge always occurs after the outgoing one in the PD-code (edge 2 in the first picture below and edge 1 in the second).
Thus, this corresponds to the fact that in the first case
a
andb
leave the crossingsC1
andC2
(in the original link) whereas they arrive there in the second case.Another way to fix the issue would be to remove all loops from the diagram before starting the Vogel algorithm. But this would harm regular isotopy which might violate the expectation of the user. Thus, I implement this as an optional feature. This gives the user the possibility to obtain a braid of smaller index if he is fine with ambient isotopy.
📝 Checklist
⌛ Dependencies