Opened 11 years ago

Closed 8 years ago

#9290 closed enhancement (fixed)

Implement Coxeter groups in their geometric representation

Reported by: nthiery Owned by: sage-combinat
Priority: major Milestone: sage-5.13
Component: combinatorics Keywords: coxeter
Cc: sage-combinat Merged in: sage-5.13.beta2
Authors: Travis Scrimshaw Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #15204 Stopgaps:

Status badges

Description (last modified by tscrim)

The root system / coxeter group code is designed from the ground up to allow for this extension.

Steps:

  • Double check CartanType(["H",3]).coxeter_diagram() and friends
  • Given a coxeter diagram, construct the dynkin diagram g corresponding to the geometric representation; most of the time, this will involve roots of unity, and require e.g. a cyclotomic field (see also #8327)
  • Make sure that L = RootSystem(g).root_space() accepts such a diagram
  • Make sure that WeylGroup(L) accepts such a root space
  • Fix all the interfaces to properly reflect the generalization (e.g. WeylGroup? above should really be CoxeterGroup?).

Apply:

Attachments (2)

trac_9290-geometric_coxeter_groups-ts.patch (33.0 KB) - added by tscrim 8 years ago.
trac-9290-review.patch (6.4 KB) - added by chapoton 8 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 11 years ago by nthiery

Partially depends on #8237

comment:2 Changed 11 years ago by nthiery

  • Description modified (diff)

comment:3 Changed 9 years ago by chapoton

  • Keywords coxeter added

comment:4 Changed 8 years ago by chapoton

see also the code in the ticket #14816

comment:5 Changed 8 years ago by tscrim

  • Status changed from new to needs_review

Here's a patch which should take care of all except the second point since I thought Dynkin diagrams where separate from Coxeter diagrams except when there are 1,2,3, or 4 bonds (and even then, we're ignoring the arrows).

Base patch is from #15137, but now uses the UCF as the default field.

comment:6 Changed 8 years ago by chapoton

  • Milestone changed from sage-wishlist to sage-5.12

comment:7 Changed 8 years ago by chapoton

  • Status changed from needs_review to needs_work

there are some failing doctests

EDIT: hmm, it rather seems to be a problem with the patchbot..

Last edited 8 years ago by chapoton (previous) (diff)

comment:8 Changed 8 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Status changed from needs_work to needs_review

Yep, something is up with the patchbot, so I gave it a kick.

sage -t ../combinat/root_system/cartan_matrix.py
    [97 tests, 15.77 s]
sage -t matrix_gps/coxeter_group.py
    [72 tests, 0.36 s]
sage -t ../combinat/root_system/coxeter_group.py
    [28 tests, 18.65 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 18.8 seconds
    cpu time: 26.2 seconds
    cumulative wall time: 34.8 seconds

comment:9 Changed 8 years ago by chapoton

one doctest failing

File "/home/chapoton/sage-5.12.beta5/devel/sage/sage/groups/matrix_gps/coxeter_group.py", line 278, in sage.groups.matrix_gps.coxeter_group.CoxeterMatrixGroup.__init__
Failed example:
    TestSuite(W).run(max_runs=30) # long time
Expected nothing
Got:
    Failure in _test_matrix_generators:
    Traceback (most recent call last):
      File "/home/chapoton/sage-5.12.beta5/local/lib/python2.7/site-packages/sage/misc/sage_unittest.py", line 282, in run
        test_method(tester = tester)
      File "/home/chapoton/sage-5.12.beta5/local/lib/python2.7/site-packages/sage/groups/matrix_gps/finitely_generated.py", line 376, in _test_matrix_generators
        tester.assertEqual(g.matrix(), h.matrix())
      File "cachefunc.pyx", line 1774, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (sage/misc/cachefunc.c:9546)
      File "/home/chapoton/sage-5.12.beta5/local/lib/python2.7/site-packages/sage/groups/matrix_gps/group_element.py", line 447, in matrix
        m = g.matrix(self.base_ring())
      File "element.pyx", line 1076, in sage.libs.gap.element.GapElement.matrix (sage/libs/gap/element.c:8618)
      File "element.pyx", line 1473, in sage.libs.gap.element.GapElement_Cyclotomic.sage (sage/libs/gap/element.c:10511)
      File "parent.pyx", line 761, in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:6823)
      File "misc.pyx", line 251, in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1606)
    AttributeError: 'UniversalCyclotomicField_with_category' object has no attribute '_n'
    ------------------------------------------------------------
    The following tests failed: _test_matrix_generators

comment:10 Changed 8 years ago by tscrim

  • Dependencies set to #15204

The problem was the conversion from gap's cyclotomics to sage by using the UCF. This is fixed in #15204.

sage: W = CoxeterGroup([[1,3,2],[3,1,6],[2,6,1]])
sage: W._test_matrix_generators()

comment:11 follow-up: Changed 8 years ago by chapoton

Hello, I have a few comments and questions

"assert implementation" : I think this use of assert to check input is not encouraged

"lazy_import('sage.groups.raag', 'RightAngledArtinGroup?')" : has this something to do in this ticket ?

What are the changes "sage/groups/matrix_gps/finitely_generated.py" for ?

comment:12 in reply to: ↑ 11 Changed 8 years ago by tscrim

Hey Frederic,

Replying to chapoton:

"assert implementation" : I think this use of assert to check input is not encouraged

This was a hold-over from the previous implementation, but this is definitely a good time to get rid of it. Fixed.

"lazy_import('sage.groups.raag', 'RightAngledArtinGroup?')" : has this something to do in this ticket ?

Because I didn't split it cleanly with #15137. Fixed.

What are the changes "sage/groups/matrix_gps/finitely_generated.py" for ?

I need to pass the CoxeterGroups category up during the initialization, so I had to make those changes.

Thanks for catching that,
Travis

comment:13 Changed 8 years ago by chapoton

Well, it seems that the patch triggers the import of numpy at startup. I have not tried to investigate where this happens.

comment:14 Changed 8 years ago by chapoton

  • Status changed from needs_review to needs_work

Changed 8 years ago by tscrim

comment:15 Changed 8 years ago by tscrim

  • Status changed from needs_work to needs_review

I made CoxeterMatrixGroup lazy import in to fix the numpy startup import.

Changed 8 years ago by chapoton

comment:16 Changed 8 years ago by chapoton

Hello Travis,

I have made a cosmetic review patch, that you can fold into yours if you want.

This almost looks good to go, but I was a bit disappointed when I tried:

sage: K = NumberField(x**2+5,'t')
sage: CoxeterGroup(['H',3],base_ring=K)

and it failed. If there is a way to make that work, it would be great !

comment:17 Changed 8 years ago by chapoton

ok, I can see no easy way to do that. Let us forget this for the moment.

Good to go ?

comment:18 Changed 8 years ago by tscrim

  • Description modified (diff)
  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

Hey Frederic,

I can't see an easy way to do so either. There might be a solution, but it'll probably be either complicated or cumbersome.

Thanks for doing the review,
Travis

comment:19 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.13.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.