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

Some improvements for braids computations #35214

Merged
merged 51 commits into from
Apr 1, 2023
Merged

Conversation

enriqueartal
Copy link
Contributor

@enriqueartal enriqueartal commented Feb 27, 2023

📚 Description

Fasten the computation of the left normal form of a braid, produce a shorter conjugating braid and if possible a pure conjugating braid

  • The method left_normal_form for braids was modified to be part of the general method for Artin groups. I propose to come back to the former method, it produces the same result but much faster. I added left_normal_form as a method for braid group and kept _left_normal_form_coxeter though it is not used any more for braids.
  • The method conjugating_braid produces a braid in left normal form with high powers of Delta, which can be reduce to 0 or 1.
  • If two braids are conjugated and have the same permutation, the conjugation can be realized by a pure braid. Starting from conjugating_braid a simple computation produces such a pure braid.

There are mild improvements for the treatment of braid groups.

Resolves #35213

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

"""
Return the right normal form of the braid.
r"""
A tuple of simple generators in the right normal form. The last
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could leave the original "Return the right normal form" and have your additions on the next line after a blank line.

@@ -1620,21 +1644,31 @@ def conjugating_braid(self, other):

sage: B = BraidGroup(3)
sage: a = B([2, 2, -1, -1])
sage: b = B([2, 1, 2, 1])
sage: b = B([2, 2, 2, 2, 1])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not understanding something, but you could leave the already existing tests and just add to them below.

sage: c = B([1])
sage: c.left_normal_form()
(1, s0)
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might consider adding tests for trivial examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. I completed the descriptions and added more examples.

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2023

Codecov Report

Patch coverage: 95.45% and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (c7e4d83) 88.60%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35214      +/-   ##
===========================================
- Coverage    88.62%   88.60%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398855   398894      +39     
===========================================
- Hits        353480   353452      -28     
- Misses       45375    45442      +67     
Impacted Files Coverage Δ
src/sage/groups/braid.py 96.79% <95.12%> (-0.09%) ⬇️
src/sage/groups/artin.py 95.39% <100.00%> (+0.03%) ⬆️

... and 23 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@enriqueartal
Copy link
Contributor Author

Fixes #35213

@trevorkarn
Copy link
Contributor

It would be good to have full coverage. Is it easy to see which of your additions don't have a test associated to them?

@enriqueartal
Copy link
Contributor Author

enriqueartal commented Feb 28, 2023

For left_normal_form, I copied the examples of the former method (in artin.py) and add some more examples. I could add something showing that now it is much faster if needed.
For conjugating_braid it is a mild change from the original method, I keep now the examples and I add some more to see how the conjugating braid is shorter. I have added in the last commit 56d33d8 an example of non conjugation.
For the new method pure_conjugating_braid, I have added in the same commit a couple of examples where the output is None

… the left normal form was a power of Delta

  to ensure all the factors of a product are braids
- In pure_conjugating_braid, check if the permutation of the conjugating braid is in the subgroup generated by
  the permutations of the centralizing braids of self; return None if not.
@enriqueartal
Copy link
Contributor Author

Commit 690c5d5 solves a couple of problems. When the conjugating braid was trivial, as coded I multiplied a braid and an integer yielding an error. For pure_conjugating_braid, cases where a conjugating braid exists but not a pure conjugating braid were not considered and the output was an error instead of None.

@trevorkarn
Copy link
Contributor

I won't be able to get to this until next week. If I haven't given any comments by the end of next week, please ping me again :)

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not hide other computational methods but expose both through an, e.g., algorithm argument. This is not just good for testing purposes (which you should do a more systematic comparison between the two methods), but also gives additional options to the user.

if p3 not in G.subgroup(LP):
return None
P = p3.word_problem(LP, display = False, as_list = True)
b1 = prod(LB[LP.index(G(a))] ** b for a,b in P)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if P is empty? Add a doctest checking for this corner case (if it can occur).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, if you make LP a dict from permutations to braids, this becomes faster and you can still pass it off to G.subgroup().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first question a previous if makes impossible to have P empty. I could not pass the dict to subgroup and even if it would be possible, I think I cannot pass it to word_problem

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first question a previous if makes impossible to have P empty.

I might say also the previous p3.is_one() also makes that the case. However, again this case should have a doctest.

I could not pass the dict to subgroup and even if it would be possible, I think I cannot pass it to word_problem

You can pass a dict to subgroup:

sage: G = groups.permutation.Symmetric(5)
sage: d = {G.random_element(): i for i in range(5)}
sage: d
{(1,5,2,4): 0, (1,5)(2,3): 1, (1,3,5,2): 2, (1,5,3,2): 3, (1,5)(2,4,3): 4}
sage: G.subgroup(d)
Subgroup generated by [(1,3,5,2), (1,5,3,2), (1,5)(2,3), (1,5)(2,4,3), (1,5,2,4)] of (Symmetric group of order 5! as a permutation group)

Indeed for word_problem, you cannot pass the dict, but you can do list(d), which will have a list in the same order the dict d was constructed (Python3 guarantees this).

src/sage/groups/braid.py Outdated Show resolved Hide resolved
src/sage/groups/braid.py Outdated Show resolved Hide resolved
def pure_conjugating_braid(self, other):
r"""
Return a pure conjugating braid, if it exists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a more detailed description; in particular, including the definition of a pure conjugating braid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be part of the one-line description. Spend a little bit of time/mathematical notation spelling it out a bit more.


INPUT:

- ``other`` -- the other braid to look for conjugating braid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 1695 to 1698
n = B.strands()
k = int(l[0][0] % 2)
b0 = B.delta() ** k * B(prod(B(a) for a in l[1:]))
return b0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
n = B.strands()
k = int(l[0][0] % 2)
b0 = B.delta() ** k * B(prod(B(a) for a in l[1:]))
return b0
l[0][0] %= 2
return B._element_from_libbraiding(l)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sage: b = B([2, 2, 2, 2, 1])
sage: c = b * a / b
sage: d1 = a.conjugating_braid(c)
sage: print (len(d1.Tietze()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8

Suggested change
sage: print (len(d1.Tietze()))
sage: print(len(d1.Tietze()))

and similar below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 1758 to 1759
sage: a1=B([1])
sage: a2=B([2])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP8

Suggested change
sage: a1=B([1])
sage: a2=B([2])
sage: a1 = B([1])
sage: a2 = B([2])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

B = self.parent()
n = B.strands()
G = SymmetricGroup(n)
p1 = G(self.permutation())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would only convert to G when it becomes necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I move the definition of G few lines below, and eliminate some conversions

if p1 != p2:
return None
b0 = self.conjugating_braid(other)
if not b0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not b0:
if b0 is not None:

It is better to check explicitly against None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done but putting if b0 is None

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, whoops. Thanks.

Simplify code for conjugating_braid

Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@enriqueartal
Copy link
Contributor Author

Done, with longer explanations for the commit.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM.

…l, TL_representation and links_gould_polynomial
@enriqueartal
Copy link
Contributor Author

I found some small typos in the documentation, @trevorkarn, could you take a look?

@tscrim
Copy link
Collaborator

tscrim commented Mar 26, 2023

@enriqueartal You need to take care of the invalid escape sequence by making the start of the doc string r”””; see the lint report.

@tscrim tscrim self-requested a review March 26, 2023 00:07
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: c7e4d83

A conjugating braid.

More precisely, if the output is `d`, `o` equals ``other``, and `s` equals ``self``
then `o = d^{-1} \cdot s \cdot d`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last trivial thing: This needs a blankline after it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and also style spaces all around

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of those were not PEP8 violations (higher precedence operators do not need the same spacing, which include [] and ()), and some of them actually makes the code harder to read. However, the rule was not applied consistently. I would actually just revert those additional PEP8 spacing changes as it makes the patch larger for minimal (at best) gain. Better separation of ticket purpose.

@enriqueartal
Copy link
Contributor Author

Done. I found curious that rules were not applied consistently even in the same line, but I just kept the required line. Unless some changes are asked by you or by some other person, I stop working in this PR.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think this is ready to go in!

@enriqueartal
Copy link
Contributor Author

@tscrim thanks for your help!

@vbraun vbraun merged commit 42817c2 into sagemath:develop Apr 1, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
@enriqueartal enriqueartal deleted the braids branch April 8, 2023 09:52
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2023
…ations, and a 2-generator presentation

    
<!-- Describe your changes here in detail -->
This PR is a continuation of sagemath#35214.
- Given an element of a symmetric group, the associated permutation
braid can be constructed, added in `_element_constructor`.
- The method `_standard_lift_Tietze` has been simplified (for a
permutation) and now it gives a shortest word for the lift but not
necesarily the smallest for the lexicographic order. The former code has
been commented.
- The method `presentation2gens` provides a presentation of the braid
group with two generators. The general method `epimorphism` has been
adapted to braid groups using this presentation and it is much faster.

### 📝 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#35985
Reported by: Enrique Manuel Artal Bartolo
Reviewer(s): Enrique Manuel Artal Bartolo, Travis Scrimshaw
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.

Some improvements for braids computations
7 participants