Speed up fundamental group computations#41691
Speed up fundamental group computations#41691enriqueartal wants to merge 19 commits intosagemath:developfrom
Conversation
|
Documentation preview for this PR (built with commit 7f13f67; changes) is ready! 🎉 |
|
What is behind this PR is the following problem. If we have an element |
| for gen in F.gens(): | ||
| j0 = gen.Tietze()[0] | ||
| rl = (l1,) + (gen * br).Tietze() + (-l1, -j0) | ||
| gen0 = gen | ||
| for m in rnf[: -1]: | ||
| gen0 = gen0 * B(m) |
There was a problem hiding this comment.
Why not factor out elt = prod(B(m) for m in rnf[:-1]) from the for gen in F.gens(): loop? In which case, you would do here gen0 = gen0 * elt.
Actually, is there an easy way to know what rnf[-1] is without computing the entire right normal form? If so, then you could get everything you want by having elt = br * ~rnf[-1]. Although rather than the above product, simply doing elt = br * ~rnf[-1] might be faster.
There was a problem hiding this comment.
For the first question you are right. I was copying a former strategy where breaking the braid was crucial to speed the computations, it is not so clear in this case. Nevertheless, to find the relevant power of delta involved in the braid I think one must use a normal form, which is quite a fast operation.
I will add this proposed change and explain the real reasons of these changes; I am not sure if they should be added to the documentation, maybe it is a good idea.
There was a problem hiding this comment.
For the description, just to the PR will likely be sufficient.
Thanks for the suggestion! Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
OK Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
small lint change
| for gen in F.gens(): | ||
| j0 = gen.Tietze()[0] | ||
| rl = (l1,) + (gen * br).Tietze() + (-l1, -j0) | ||
| gen0 = gen | ||
| for m in rnf[: -1]: | ||
| gen0 = gen0 * B(m) |
There was a problem hiding this comment.
For the description, just to the PR will likely be sufficient.
simplify a product
|
I try to explain the reasons for these changes. The fundamental groups computed are the quotient of a free group by some relations. For some of them, we need to compute If the length of the word defining There are closed formulas for If and the closed formulas are much faster than running along the words defined by the powers of |
|
Some mistakes produced errors in the text. In particular when the right normal form has only a power of the |
|
Thank you for the details. Seems like the tests pass here. So I should finish the review then? |
|
For me it is OK |
sagemathgh-41691: Speed up fundamental group computations <!-- ^ 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". --> The computation of plane curve complements uses mapping class group computations and this operation can be very long if attacked directly. Some changes were applied in sagemath#36768, but some computations take forever without the changes in this PR. Note. A previous version of this PR did not use the right branch ### 📝 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. - [ ] I have created tests covering the changes. - [ ] 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#41691 Reported by: Enrique Manuel Artal Bartolo Reviewer(s): Enrique Manuel Artal Bartolo, Travis Scrimshaw
sagemathgh-41691: Speed up fundamental group computations <!-- ^ 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". --> The computation of plane curve complements uses mapping class group computations and this operation can be very long if attacked directly. Some changes were applied in sagemath#36768, but some computations take forever without the changes in this PR. Note. A previous version of this PR did not use the right branch ### 📝 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. - [ ] I have created tests covering the changes. - [ ] 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#41691 Reported by: Enrique Manuel Artal Bartolo Reviewer(s): Enrique Manuel Artal Bartolo, Travis Scrimshaw
The computation of plane curve complements uses mapping class group computations and this operation can be very long if attacked directly. Some changes were applied in #36768, but some computations take forever without the changes in this PR.
Note. A previous version of this PR did not use the right branch
📝 Checklist
⌛ Dependencies