Opened 4 months ago

Closed 4 months ago

#30237 closed defect (fixed)

Make .coxeter_matrix() return a CoxeterMatrix for coxeter3-implemented groups

Reported by: gh-cemulate Owned by:
Priority: minor Milestone: sage-9.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 gh-cemulate)

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 ill-founded, 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 gh-cemulate

  • Branch set to u/gh-cemulate/coxeter3_matrix_cleanup

comment:2 Changed 4 months ago by git

  • Commit set to c1b44460b7370a21029636ada3549bffd2e217b2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c1b4446Add test

comment:3 Changed 4 months ago by gh-cemulate

  • Authors set to Chase Meadors
  • 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 coxeter3-matrix-cleanup to Make .coxeter_matrix() return a CoxeterMatrix for coxeter3-implemented groups
  • Type changed from PLEASE CHANGE to defect

New commits:

c1b4446Add test

comment:4 Changed 4 months ago by tscrim

  • 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 git

  • Commit changed from c1b44460b7370a21029636ada3549bffd2e217b2 to d5a74739c42f41fbdb6c71c5d87f82c71f8e5786

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

d5a7473Add deprecation for removed method.

comment:6 Changed 4 months ago by gh-cemulate

Done.

comment:7 Changed 4 months ago by tscrim

It needs to be doctested with a docstring as well.

comment:8 Changed 4 months ago by git

  • Commit changed from d5a74739c42f41fbdb6c71c5d87f82c71f8e5786 to bc2e1d1f32ee16f929a794b048d445326159cf33

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

bc2e1d1Add doctests for deprecated method.

comment:9 Changed 4 months ago by gh-cemulate

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 tscrim

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 git

  • Commit changed from bc2e1d1f32ee16f929a794b048d445326159cf33 to 86fd6ec2727d5c7df74f5050fbf299779e2e49b5

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

86fd6ecAdd docstring for deprecated method.

comment:12 Changed 4 months ago by gh-cemulate

Done.

comment:13 Changed 4 months ago by git

  • Commit changed from 86fd6ec2727d5c7df74f5050fbf299779e2e49b5 to c55ec05ec3780e9d0c271035bedfd199bacbe677

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

c55ec05Mark other doctest line optional.

comment:14 Changed 4 months ago by tscrim

Sorry, one last thing. It needs to actually have the same result/behavior as before.

comment:15 Changed 4 months ago by git

  • Commit changed from c55ec05ec3780e9d0c271035bedfd199bacbe677 to bd9b5c661d13f4c37a7c72430603b3ce92f0b5d2

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

bd9b5c6Make deprecated method have same behavior.

comment:16 Changed 4 months ago by gh-cemulate

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 tscrim

  • Status changed from needs_review to positive_review

Perfect, thank you.

comment:18 Changed 4 months ago by vbraun

  • Branch changed from u/gh-cemulate/coxeter3_matrix_cleanup to bd9b5c661d13f4c37a7c72430603b3ce92f0b5d2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.