Opened 2 years ago
Closed 19 months ago
#27302 closed enhancement (fixed)
Cubic Braid Groups
Reported by:  soehms  Owned by:  

Priority:  major  Milestone:  sage9.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, GitHub, GitLab)  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)
Change History (40)
comment:1 Changed 2 years ago by
 Branch set to u/soehms/cubic_braid_group_27302
comment:2 Changed 2 years ago by
 Commit set to 756a7bbdf4d86553a99b298cfa11933c0ca8f423
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Commit changed from 756a7bbdf4d86553a99b298cfa11933c0ca8f423 to 623e14b4f5d9cbfb852ac1702c028db80f291ef5
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
80aabe6  25894: implement reviewers suggestions

1997ad7  25894: missing longdoctest fixed, as well

49a04af  25894: restore lost changes

c7cb49f  Merge branch 'u/soehms/matrix_groups_subgroups25894' of git://trac.sagemath.org/sage into matrix_group_subgroups_25894

0aec3cc  change representation strings according to reviewers suggestion

455bd17  fixing further doctests

c1280b9  25894: corrections on doctest layout

ea70462  Merge commit 'c1280b9fbd954c5f43be122cafd4606872ed353c' of git://trac.sagemath.org/sage into cubic_braid_group_27302

b29a9fc  Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

623e14b  PEP8 adaptions

comment:4 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.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 2 years ago by
 Commit changed from 623e14b4f5d9cbfb852ac1702c028db80f291ef5 to 6b638b40887f06a54551afa52b1c273f793c61e1
Branch pushed to git repo; I updated commit sha1. New commits:
6b638b4  Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:7 Changed 2 years ago by
 Commit changed from 6b638b40887f06a54551afa52b1c273f793c61e1 to 76a845e05e5ffe45b2f0203a6076691640e8b8bf
Branch pushed to git repo; I updated commit sha1. New commits:
76a845e  27302: fixed doctests

comment:8 Changed 2 years ago by
 Commit changed from 76a845e05e5ffe45b2f0203a6076691640e8b8bf to 3402ba75d5159fb4f8b1c4f94a6d7ac25bee99ed
Branch pushed to git repo; I updated commit sha1. New commits:
3402ba7  27302: python3 fixes

comment:9 Changed 2 years ago by
 Commit changed from 3402ba75d5159fb4f8b1c4f94a6d7ac25bee99ed to 4b967409c6e7be2871f355ce370c19cb0211a815
Branch pushed to git repo; I updated commit sha1. New commits:
4b96740  27302: further corrections according to patchbot hints

comment:10 followup: ↓ 12 Changed 2 years ago by
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, n3) >>> range(n  3)
In
+ if m % 2 == 0: + mhalf = m/2 + else: + mhalf = (m1)/2
rather use exact division //
comment:11 Changed 2 years ago by
 Commit changed from 4b967409c6e7be2871f355ce370c19cb0211a815 to 1164156b48655b2ef30ec2ef877757a4ee211740
Branch pushed to git repo; I updated commit sha1. New commits:
1164156  27302: typo and style fixes

comment:12 in reply to: ↑ 10 Changed 2 years ago by
comment:13 followup: ↓ 15 Changed 2 years ago by
two more:
typo: hermitain
typo: performace
comment:14 Changed 2 years ago by
 Commit changed from 1164156b48655b2ef30ec2ef877757a4ee211740 to 46a0134f143615c55c2e436567950517252f960c
Branch pushed to git repo; I updated commit sha1. New commits:
46a0134  27302: spellcheck performed

comment:15 in reply to: ↑ 13 Changed 2 years ago by
comment:16 followup: ↓ 17 Changed 2 years ago by
From my first lookthrough, 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 toplevel, which means it should not be a parameter ofCubicBraidGroup
.  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 ; followup: ↓ 18 Changed 2 years ago by
Replying to tscrim:
From my first lookthrough, 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, andif 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 toplevel, which means it should not be a parameter ofCubicBraidGroup
.
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, Cdriven 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 2 years ago by
Replying to soehms:
Replying to tscrim:
From my first lookthrough, 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, andif 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 toplevel, which means it should not be a parameter ofCubicBraidGroup
.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, Cdriven opinion, but I'm still convinced that such code is not really save. So I changed this sinceEnum
is available, right now. By the functionsAssionGroupU/S
there is no need for the user to have theEnum
externally.
I go backandforth 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 toplevel 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 maintenancewise as the toplevel 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 2 years ago by
 Commit changed from 46a0134f143615c55c2e436567950517252f960c to 46840491603debc1785300287cbb6933c144fdf7
Changed 2 years ago by
comment:20 Changed 2 years ago by
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 constructionfunctions.
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:
 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).  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 keystrokes 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 2 years ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:22 Changed 2 years ago by
 Commit changed from 46840491603debc1785300287cbb6933c144fdf7 to 44446cb58f0b710f923dfdb7dcd34c0daaae58a6
Branch pushed to git repo; I updated commit sha1. New commits:
44446cb  Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_groups_27302

comment:23 Changed 2 years ago by
 Commit changed from 44446cb58f0b710f923dfdb7dcd34c0daaae58a6 to 24c1d068c530ec9b8ebd9fd1283a02a1906d7463
Branch pushed to git repo; I updated commit sha1. New commits:
24c1d06  27302: merge conflict fixed again and assert_ > assertTrue

comment:24 Changed 2 years ago by
 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 2 years ago by
 Milestone changed from sage8.8 to sage8.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 23 months ago by
 Commit changed from 24c1d068c530ec9b8ebd9fd1283a02a1906d7463 to fe4adff9d357284a8cf57ad8aaace92488a6a475
comment:27 Changed 23 months ago by
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 Buraumatrix of the cubic braid.
The default setting for the characteristic of the Buraumatrix was implemented in the as_matrix_group
method, which is not the right place. I fixed that, as well.
comment:28 Changed 22 months ago by
patchbot's pyflakesplugin reports an unused import
comment:29 Changed 21 months ago by
 Commit changed from fe4adff9d357284a8cf57ad8aaace92488a6a475 to 4ea92635c4a7df9872e931f1937238b13ed5d4fd
Branch pushed to git repo; I updated commit sha1. New commits:
4ea9263  Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:30 Changed 21 months ago by
 Commit changed from 4ea92635c4a7df9872e931f1937238b13ed5d4fd to 6663fa7c0f694f556d8204f8b43265e63101794c
Branch pushed to git repo; I updated commit sha1. New commits:
6663fa7  27302: add methods __init__, __hash__,and _matrix_ to the element class

comment:31 Changed 21 months ago by
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 ofburau_matrix
with better performance properties.  add method
__init__
to the element class to perform a reduction on the Tietzetuple.
comment:32 Changed 20 months ago by
 Commit changed from 6663fa7c0f694f556d8204f8b43265e63101794c to db5b7a7e0e6d94d15d545435c1efeb7d5cf9781a
comment:33 Changed 20 months ago by
 Milestone changed from sage8.9 to sage9.0
comment:34 Changed 20 months ago by
 Dependencies #25894 deleted
 Status changed from needs_review to needs_work
now the branch is red
comment:35 Changed 20 months ago by
 Commit changed from db5b7a7e0e6d94d15d545435c1efeb7d5cf9781a to 9fdafdde918173d494df397683de6a8c55fa67f1
Branch pushed to git repo; I updated commit sha1. New commits:
9fdafdd  Merge branch 'u/soehms/cubic_braid_group_27302' of git://trac.sagemath.org/sage into cubic_braid_group_27302

comment:36 Changed 20 months ago by
 Status changed from needs_work to needs_review
Merge error in references index.rst fixed
comment:37 Changed 19 months ago by
 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 19 months ago by
Thanks!
comment:39 Changed 19 months ago by
 Branch changed from u/soehms/cubic_braid_group_27302 to 9fdafdde918173d494df397683de6a8c55fa67f1
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
implement subgroup / ambient methods for MatrixGroup_base
removed unused keyword
Merge branch 'u/soehms/matrix_groups_subgroups25894' of git://trac.sagemath.org/sage into cubic_braid_group
27302: initial version