Opened 10 months ago

Closed 3 weeks ago

#27302 closed enhancement (fixed)

Cubic Braid Groups

Reported by: soehms Owned by:
Priority: major Milestone: sage-9.0
Component: group theory Keywords: braid groups, complex reflection groups
Cc: tscrim Merged in:
Authors: Sebastian Oehms Reviewers: Travis Scrimshaw, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 9fdafdd (Commits) Commit: 9fdafdde918173d494df397683de6a8c55fa67f1
Dependencies: Stopgaps:

Description

This ticket implements a new class to work with factor groups of the Artin braid groups having order three on the braid generators.

Attachments (1)

CubicBraidGroupType.png (14.5 KB) - added by soehms 8 months ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 10 months ago by soehms

  • Branch set to u/soehms/cubic_braid_group_27302

comment:2 Changed 10 months ago by soehms

  • Commit set to 756a7bbdf4d86553a99b298cfa11933c0ca8f423
  • Status changed from new to needs_review

New commits:

0133335implement subgroup / ambient methods for MatrixGroup_base
747bc16removed unused keyword
d36aeb8Merge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into cubic_braid_group
756a7bb27302: initial version

comment:3 Changed 9 months ago by git

  • Commit changed from 756a7bbdf4d86553a99b298cfa11933c0ca8f423 to 623e14b4f5d9cbfb852ac1702c028db80f291ef5

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

80aabe625894: implement reviewers suggestions
1997ad725894: missing long-doctest fixed, as well
49a04af25894: restore lost changes
c7cb49fMerge branch 'u/soehms/matrix_groups_subgroups-25894' of git://trac.sagemath.org/sage into matrix_group_subgroups_25894
0aec3ccchange representation strings according to reviewers suggestion
455bd17fixing further doctests
c1280b925894: corrections on doctest layout
ea70462Merge commit 'c1280b9fbd954c5f43be122cafd4606872ed353c' of git://trac.sagemath.org/sage into cubic_braid_group_27302
b29a9fcMerge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302
623e14bPEP8 adaptions

comment:4 Changed 9 months ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:5 Changed 9 months ago by git

  • Commit changed from 623e14b4f5d9cbfb852ac1702c028db80f291ef5 to 6b638b40887f06a54551afa52b1c273f793c61e1

Branch pushed to git repo; I updated commit sha1. New commits:

6b638b4Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:6 Changed 9 months ago by soehms

  • Authors set to Sebastian Oehms

Fixed merge conflict!

comment:7 Changed 9 months ago by git

  • Commit changed from 6b638b40887f06a54551afa52b1c273f793c61e1 to 76a845e05e5ffe45b2f0203a6076691640e8b8bf

Branch pushed to git repo; I updated commit sha1. New commits:

76a845e27302: fixed doctests

comment:8 Changed 9 months ago by git

  • Commit changed from 76a845e05e5ffe45b2f0203a6076691640e8b8bf to 3402ba75d5159fb4f8b1c4f94a6d7ac25bee99ed

Branch pushed to git repo; I updated commit sha1. New commits:

3402ba727302: python3 fixes

comment:9 Changed 8 months ago by git

  • Commit changed from 3402ba75d5159fb4f8b1c4f94a6d7ac25bee99ed to 4b967409c6e7be2871f355ce370c19cb0211a815

Branch pushed to git repo; I updated commit sha1. New commits:

4b9674027302: further corrections according to patchbot hints

comment:10 follow-up: Changed 8 months ago by chapoton

Some random remarks:

typo "refelection"

typo "Vancover"

burau >>> Burau

typo "availlable"

typo "as_permutatopn_group"

Use "Construct" and "Return" in the first lines of doc, not "Constructs" and "Returns"

"len(root_list) == 0" could be "not(root_list)" which is faster

+    TESTS:
+
+        sage: C3 = CubicBraidGroup(3)

is missing a double ::

range(0, n-3) >>> range(n - 3)

In

+            if m % 2  == 0:
+                mhalf = m/2
+            else:
+                mhalf = (m-1)/2

rather use exact division //

comment:11 Changed 8 months ago by git

  • Commit changed from 4b967409c6e7be2871f355ce370c19cb0211a815 to 1164156b48655b2ef30ec2ef877757a4ee211740

Branch pushed to git repo; I updated commit sha1. New commits:

116415627302: typo and style fixes

comment:12 in reply to: ↑ 10 Changed 8 months ago by soehms

Replying to chapoton:

Some random remarks: ...

Thanks for having a look at this!

comment:13 follow-up: Changed 8 months ago by chapoton

two more:

typo: hermitain

typo: performace

comment:14 Changed 8 months ago by git

  • Commit changed from 1164156b48655b2ef30ec2ef877757a4ee211740 to 46a0134f143615c55c2e436567950517252f960c

Branch pushed to git repo; I updated commit sha1. New commits:

46a013427302: spell-check performed

comment:15 in reply to: ↑ 13 Changed 8 months ago by soehms

Replying to chapoton:

two more:

typo: hermitain

typo: performace

Thanks again!

comment:16 follow-up: Changed 8 months ago by tscrim

From my first look-through, the code looks to be quite good. I have a lot of little comments that are going to be better handled as a push from me, but mostly they revolve around docstring formatting, not using <>, PEP8, and if len(foo) == 0: -> if not foo: (if you want to hunt them down and take care of them). Some more substantial comments are:

  • A number of methods do not have doctests.
  • I am tempted to get rid of the Enum, but at least I don't think it should be accessible from the top-level, which means it should not be a parameter of CubicBraidGroup.
  • I am inclined to put CubicBraidGroup as the class with a __classcall_private__ construction, which in some ways would contradict the previous comment. I might have to think more about this.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 months ago by soehms

Replying to tscrim:

From my first look-through, the code looks to be quite good. I have a lot of little comments that are going to be better handled as a push from me, but mostly they revolve around docstring formatting, not using <>, PEP8, and if len(foo) == 0: -> if not foo: (if you want to hunt them down and take care of them). Some more substantial comments are:

  • A number of methods do not have doctests.

It seems that you didn't look at the most recent commit. I did some corrections last week after inspecting the patchbot results more carefully and following Frédéric Chapoton's hints. So, I think your comments above should be fixed, already.

  • I am tempted to get rid of the Enum, but at least I don't think it should be accessible from the top-level, which means it should not be a parameter of CubicBraidGroup.

In my first version (under Sage 6.9, the one I pushed to CoCalc?) I didn't use Enum since it was not accessible at that time. I'm used to avoid creating code that depends on literals. Maybe this is an old fashioned, C-driven opinion, but I'm still convinced that such code is not really save. So I changed this since Enum is available, right now. By the functions AssionGroupU/S there is no need for the user to have the Enum externally.

On the other hand I realized that it is still unusual to use Enum in Sage (there is just one module (modular_decomposition.py) doing that). Therefore, I'm open to change this again.

  • I am inclined to put CubicBraidGroup as the class with a __classcall_private__ construction, which in some ways would contradict the previous comment. I might have to think more about this.

As far as I understand that construction, this would replace the three functions to create an instance of the class, right? I implemented the construction of instances according to the braid.py module. What is the advantage to use __classcall_private__ instead? I would appreciate to learn more about that!

comment:18 in reply to: ↑ 17 Changed 8 months ago by tscrim

Replying to soehms:

Replying to tscrim:

From my first look-through, the code looks to be quite good. I have a lot of little comments that are going to be better handled as a push from me, but mostly they revolve around docstring formatting, not using <>, PEP8, and if len(foo) == 0: -> if not foo: (if you want to hunt them down and take care of them). Some more substantial comments are:

  • A number of methods do not have doctests.

It seems that you didn't look at the most recent commit. I did some corrections last week after inspecting the patchbot results more carefully and following Frédéric Chapoton's hints. So, I think your comments above should be fixed, already.

I was pretty sure I was looking at the most recent commit because I clicked on the branch name. Although with beta1 released, there appears to be another (certainly trivial) merge conflict.

  • I am tempted to get rid of the Enum, but at least I don't think it should be accessible from the top-level, which means it should not be a parameter of CubicBraidGroup.

In my first version (under Sage 6.9, the one I pushed to CoCalc?) I didn't use Enum since it was not accessible at that time. I'm used to avoid creating code that depends on literals. Maybe this is an old fashioned, C-driven opinion, but I'm still convinced that such code is not really save. So I changed this since Enum is available, right now. By the functions AssionGroupU/S there is no need for the user to have the Enum externally.

I go back-and-forth on the Enum. If this was C/C++, I would say definitely use it (and I do like the paradigm overall). The problem is in Python, it means we have to explicitly import it, and we have to put it into the global namespace if we want it to be top-level accessible to the user. As you said, with AssionGroupU/S (which does serve a role in the global namespace) we can avoid this. So maybe this is not a problem as long as we hide it completely from the (standard) user.

  • I am inclined to put CubicBraidGroup as the class with a __classcall_private__ construction, which in some ways would contradict the previous comment. I might have to think more about this.

As far as I understand that construction, this would replace the three functions to create an instance of the class, right? I implemented the construction of instances according to the braid.py module. What is the advantage to use __classcall_private__ instead? I would appreciate to learn more about that!

__classcall_private__ essentially allows you to do extra processing on the inputs before getting to the __init__ method. Thus we can actually return other parents/instances/objects/etc. if we deem the inputs worthy. Thus we can simply replace the CubicBraidGroup function with CubicBraidGroup_class (renamed to CubicBraidGroup), making it easier maintenance-wise as the top-level object is the class itself.

Although maybe we don't even need such a heavy tool as the CubicBraidGroup_class.__init__ does the job, and we just have the default being the Enum for the standard cubic braid group. I think this might actually be sufficient.

comment:19 Changed 8 months ago by git

  • Commit changed from 46a0134f143615c55c2e436567950517252f960c to 46840491603debc1785300287cbb6933c144fdf7

Branch pushed to git repo; I updated commit sha1. New commits:

904fde1Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_groups_27302
468404927302: implement __classcall_private__ and move Enum into the class

Changed 8 months ago by soehms

comment:20 Changed 8 months ago by soehms

Thanks for your explanations, Travis! Indeed, I had no good feeling to include the Enum in the global namespace. So, I encoded the type into the names of the construction-functions. But, this would not be an option for longer list of types (and is structurally not really nice).

Therefore, I now try the following solution:

  1. I move the Enum into the class as a member, so that it is implicitly in the global namespace (via the name of the class).
  2. I make the function CubicBraidGroup be the __classcall_private__ method and rename the class accordingly as you suggested.

I keep the functions AssionGroupS/U for convenience even though there aren't necessary any more (in order to capsule the import of the Enum). The remaining purpose of them is to reduce key-strokes needed to create the corresponding instances. But, in principle they could be removed.

Compared to the use of literals the advantage of the Enum is not only the paradigm, but also the possibility for the user to select the type from the list:

comment:21 Changed 7 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:22 Changed 6 months ago by git

  • Commit changed from 46840491603debc1785300287cbb6933c144fdf7 to 44446cb58f0b710f923dfdb7dcd34c0daaae58a6

Branch pushed to git repo; I updated commit sha1. New commits:

44446cbMerge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_groups_27302

comment:23 Changed 6 months ago by git

  • Commit changed from 44446cb58f0b710f923dfdb7dcd34c0daaae58a6 to 24c1d068c530ec9b8ebd9fd1283a02a1906d7463

Branch pushed to git repo; I updated commit sha1. New commits:

24c1d0627302: merge conflict fixed again and assert_ -> assertTrue

comment:24 Changed 6 months ago by soehms

  • Status changed from needs_work to needs_review

I set needs_review again, even though the optional gap3 doctests fail under python3. But this seems to be another problem:

sage: from sage.interfaces.gap3 import gap3
sage: gap3._start()
sage: gap3.load_package("chevie")
Traceback (most recent call last):
...
RuntimeError: startswith first arg must be bytes or a tuple of bytes, not str

Is this a known issue?

comment:25 Changed 5 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

comment:26 Changed 4 months ago by git

  • Commit changed from 24c1d068c530ec9b8ebd9fd1283a02a1906d7463 to fe4adff9d357284a8cf57ad8aaace92488a6a475

Branch pushed to git repo; I updated commit sha1. New commits:

8375891Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_groups_27302
fe4adff27302: comparison for larger groups fixed

comment:27 Changed 4 months ago by soehms

While developing a class for cubic Hecke algebras I observed that comparison for cubic braids on more than 5 strands didn't terminate (since the implementation from the finitely presented groups was used). So, I replaced that comparison for the groups on more than 5 strands by comparison of the corresponding braids resp. the Burau-matrix of the cubic braid.

The default setting for the characteristic of the Burau-matrix was implemented in the as_matrix_group method, which is not the right place. I fixed that, as well.

comment:28 Changed 4 months ago by chapoton

patchbot's pyflakes-plugin reports an unused import

comment:29 Changed 3 months ago by git

  • Commit changed from fe4adff9d357284a8cf57ad8aaace92488a6a475 to 4ea92635c4a7df9872e931f1937238b13ed5d4fd

Branch pushed to git repo; I updated commit sha1. New commits:

4ea9263Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:30 Changed 3 months ago by git

  • Commit changed from 4ea92635c4a7df9872e931f1937238b13ed5d4fd to 6663fa7c0f694f556d8204f8b43265e63101794c

Branch pushed to git repo; I updated commit sha1. New commits:

6663fa727302: add methods __init__, __hash__,and _matrix_ to the element class

comment:31 Changed 3 months ago by soehms

I do the following:

  • fix pyflakes
  • add method __hash__ to the element class, since the inherited version is not well defined
  • add method _matrix_ to the element class as an alternative to the default of burau_matrix with better performance properties.
  • add method __init__ to the element class to perform a reduction on the Tietze-tuple.

comment:32 Changed 8 weeks ago by git

  • Commit changed from 6663fa7c0f694f556d8204f8b43265e63101794c to db5b7a7e0e6d94d15d545435c1efeb7d5cf9781a

Branch pushed to git repo; I updated commit sha1. New commits:

2de6f71Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302
db5b7a727302: some cleaning in burau_matrix

comment:33 Changed 8 weeks ago by soehms

  • Milestone changed from sage-8.9 to sage-9.0

comment:34 Changed 5 weeks ago by chapoton

  • Dependencies #25894 deleted
  • Status changed from needs_review to needs_work

now the branch is red

comment:35 Changed 5 weeks ago by git

  • Commit changed from db5b7a7e0e6d94d15d545435c1efeb7d5cf9781a to 9fdafdde918173d494df397683de6a8c55fa67f1

Branch pushed to git repo; I updated commit sha1. New commits:

9fdafddMerge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:36 Changed 5 weeks ago by soehms

  • Status changed from needs_work to needs_review

Merge error in references index.rst fixed

comment:37 Changed 4 weeks ago by chapoton

  • Reviewers set to Travis Scrimshaw, Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be. One can keep further enhancement for later tickets.

comment:38 Changed 4 weeks ago by soehms

Thanks!

comment:39 Changed 3 weeks ago by vbraun

  • Branch changed from u/soehms/cubic_braid_group_27302 to 9fdafdde918173d494df397683de6a8c55fa67f1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.