Opened 4 months ago
Closed 4 months ago
#30237 closed defect (fixed)
Make .coxeter_matrix() return a CoxeterMatrix for coxeter3implemented groups
Reported by:  ghcemulate  Owned by:  

Priority:  minor  Milestone:  sage9.2 
Component:  combinatorics  Keywords:  CoxeterGroup, CoxeterMatrix, coxeter, coxeter3 
Cc:  Merged in:  
Authors:  Chase Meadors  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  bd9b5c6 (Commits)  Commit:  bd9b5c661d13f4c37a7c72430603b3ce92f0b5d2 
Dependencies:  Stopgaps: 
Description (last modified by )
This patch fixes all of the following, which currently throw errors:
W = CoxeterGroup(['B', 3], implementation='coxeter3') W.coxeter_type() # AttributeError: 'sage.matrix.matrix_integer_dense.Matrix_integer_dense' object has no attribute 'coxeter_type' W([1,2,1]).reduced_words() # IndexError: matrix index out of range R.<v> = LaurentPolynomialRing(ZZ) IwahoriHeckeAlgebra(W, v, 1/v) # AttributeError: 'sage.matrix.matrix_integer_dense.Matrix_integer_dense' object has no attribute 'coxeter_type'
The underlying problem in all cases is that W.coxeter_matrix()
does not return a CoxeterMatrix
; I've altered this to return the (correctly indexed) coxeter matrix and added a test.
This also removes a completely unused function CoxeterGroup.m(i, j)
which seemingly existed only to workaround the fact that .coxeter_matrix()
was incorrectly indexed (and in fact is illfounded, it would cause an error on a group with affine Cartan type). This removal breaks no tests in libs/coxeter3
.
Change History (18)
comment:1 Changed 4 months ago by
 Branch set to u/ghcemulate/coxeter3_matrix_cleanup
comment:2 Changed 4 months ago by
 Commit set to c1b44460b7370a21029636ada3549bffd2e217b2
comment:3 Changed 4 months ago by
 Component changed from PLEASE CHANGE to combinatorics
 Description modified (diff)
 Keywords CoxeterGroup CoxeterMatrix coxeter coxeter3 added
 Priority changed from major to minor
 Status changed from new to needs_review
 Summary changed from coxeter3matrixcleanup to Make .coxeter_matrix() return a CoxeterMatrix for coxeter3implemented groups
 Type changed from PLEASE CHANGE to defect
New commits:
c1b4446  Add test

comment:4 Changed 4 months ago by
 Reviewers set to Travis Scrimshaw
The m()
method might not be used in the code, but some user might be using it in their personal code. You should issue a deprecation instead with the appropriate redirect:
from sage.misc.superseded import deprecation deprecation(30237, "the .m(i, j) method has been deprecated; instead use .coxeter_matrix()[i,j]")
or whatever message is appropriate.
Other than that, LGTM.
comment:5 Changed 4 months ago by
 Commit changed from c1b44460b7370a21029636ada3549bffd2e217b2 to d5a74739c42f41fbdb6c71c5d87f82c71f8e5786
Branch pushed to git repo; I updated commit sha1. New commits:
d5a7473  Add deprecation for removed method.

comment:6 Changed 4 months ago by
Done.
comment:7 Changed 4 months ago by
It needs to be doctested with a docstring as well.
comment:8 Changed 4 months ago by
 Commit changed from d5a74739c42f41fbdb6c71c5d87f82c71f8e5786 to bc2e1d1f32ee16f929a794b048d445326159cf33
Branch pushed to git repo; I updated commit sha1. New commits:
bc2e1d1  Add doctests for deprecated method.

comment:9 Changed 4 months ago by
Ah, sorry, wasn't sure if that needed to be done in this case. It's there now.
comment:10 Changed 4 months ago by
Can you quickly add a docstring just saying This is deprecated, use ``self.coxeter_matrix()[i,j]`` instead.
?
comment:11 Changed 4 months ago by
 Commit changed from bc2e1d1f32ee16f929a794b048d445326159cf33 to 86fd6ec2727d5c7df74f5050fbf299779e2e49b5
Branch pushed to git repo; I updated commit sha1. New commits:
86fd6ec  Add docstring for deprecated method.

comment:12 Changed 4 months ago by
Done.
comment:13 Changed 4 months ago by
 Commit changed from 86fd6ec2727d5c7df74f5050fbf299779e2e49b5 to c55ec05ec3780e9d0c271035bedfd199bacbe677
Branch pushed to git repo; I updated commit sha1. New commits:
c55ec05  Mark other doctest line optional.

comment:14 Changed 4 months ago by
Sorry, one last thing. It needs to actually have the same result/behavior as before.
comment:15 Changed 4 months ago by
 Commit changed from c55ec05ec3780e9d0c271035bedfd199bacbe677 to bd9b5c661d13f4c37a7c72430603b3ce92f0b5d2
Branch pushed to git repo; I updated commit sha1. New commits:
bd9b5c6  Make deprecated method have same behavior.

comment:16 Changed 4 months ago by
I hope I understood correctly; I made it have the same behavior as before by "fixing" it; should it instead have the same implementation as before (which would cause the old doctests to throw errors)? Thanks for guiding me through the protocol here, by the way.
comment:17 Changed 4 months ago by
 Status changed from needs_review to positive_review
Perfect, thank you.
comment:18 Changed 4 months ago by
 Branch changed from u/ghcemulate/coxeter3_matrix_cleanup to bd9b5c661d13f4c37a7c72430603b3ce92f0b5d2
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Add test