Opened 6 years ago

Closed 15 months ago

#11010 closed enhancement (fixed)

Implementation of the SubwordComplex as defined by Knutson and Miller

Reported by: stumpc5 Owned by: tbd
Priority: major Milestone: sage-7.0
Component: combinatorics Keywords: subword complex, simplicial complex
Cc: Merged in:
Authors: Christian Stump Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 17518c1 (Commits) Commit: 17518c11f06b77c1823a5b0950285085540a41b2
Dependencies: Stopgaps:

Description (last modified by stumpc5)

This patch provides an implementation of the subword complex:

Fix a Coxeter system (W,S). Let Q = be a finite word in S and pi in W.

The subword complex Delta(Q,pi) is then defined to be the simplicial complex with vertices being {0,...,n-1}, (n = len(Q), one vertex for each letter in Q) and with facets given by all (indices of) subwords Q' of Q for which Q\Q' is a reduced expression for pi.

    sage: W = CoxeterGroup(['A',2],index_set=[1,2])
    sage: w = W.from_reduced_word([1,2,1])
    sage: C = SubwordComplex([2,1,2,1,2],w); C
    Subword complex of type ['A', 2] for Q = [2, 1, 2, 1, 2] and pi = 121
    sage: C.facets()
    {(1, 2), (3, 4), (0, 4), (2, 3), (0, 1)}

Attachments (1)

trac_11010-subword_complex-cs.patch (35.1 KB) - added by stumpc5 4 years ago.

Download all attachments as: .zip

Change History (83)

comment:1 Changed 6 years ago by stumpc5

  • Authors set to Christian Stump
  • Component changed from PLEASE CHANGE to combinatorics
  • Description modified (diff)
  • Keywords subword complex simplicial complex added
  • Status changed from new to needs_work
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 6 years ago by stumpc5

  • Dependencies set to 11187, 11122
  • Description modified (diff)

comment:3 Changed 6 years ago by stumpc5

  • Dependencies changed from 11187, 11122 to #11187, #11122

comment:4 Changed 6 years ago by stumpc5

  • Milestone set to sage-4.7.2

comment:5 Changed 5 years ago by stumpc5

  • Dependencies changed from #11187, #11122 to #11122

comment:6 follow-up: Changed 4 years ago by chapoton

  • Dependencies changed from #11122 to #12774
  • Status changed from needs_work to needs_review

comment:7 in reply to: ↑ 6 Changed 4 years ago by stumpc5

  • Dependencies changed from #12774 to #11187, #12774

Replying to chapoton:

Okay, I must confess that I tend to not finish implementations once the patch does what I want it to do...

The dependencies of this patch must (unfortunately) inherit as well the dependencies of #11122. This is to say that this patch actually relies on many things implemented in #11187.

I would be very, very glad if anyone would finally start reviewing the universal cyclotomic field, #8327 on which #11187 depends (I would first need to rebase it, and make sure that the trac version is updated to the newest version on the combinat queue).

Cheers, Christian

Changed 4 years ago by stumpc5

comment:8 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/11010
  • Commit set to a525fdcf47ead6e1794c4f54acac00ee70f07e91

I have made a git branch. It does not work: runs into an infinite recursion because of #15456.


New commits:

3b8df81trac_11010-subword_complex-cs.patch
a525fdctrac #11010 code clean-up

comment:11 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work
  • Work issues set to coverage

The code needs to be 100% doctested. Some empty methods should be removed.

comment:12 Changed 3 years ago by chapoton

  • Dependencies changed from #11187, #12774 to #11187, #12774, #15456

Hello Christian, do you really think that this ticket depends on #11187 ?

I am tempted to ressurect this one, without waiting for the famous category-and-axiom ticket.

With not so much work, I managed to get a branch with only 19 failures.

comment:13 Changed 3 years ago by git

  • Commit changed from a525fdcf47ead6e1794c4f54acac00ee70f07e91 to 76afdca9d2842a4f0a4060f60130bb37c41df99f

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

4f3945dtrac_11010-subword_complex-cs.patch
e77f24dtrac #11010 code clean-up
0599a0atrac #11010 has_descent is better
76afdcatrac #11010 more corrections, and rebased

comment:14 Changed 3 years ago by chapoton

  • Dependencies changed from #11187, #12774, #15456 to #12774

Here is a better branch.

I am removing the dependencies to #11187, #15456 as I think they are not really needed.

There are still 16 failing doctests. Work in progress.

comment:15 Changed 3 years ago by stumpc5

Dear Frederic,

Thanks for your work on that -- I am super busy with other stuff, hopefully this will get better in about 4 weeks from now.

I am not sure if I/we should keep the construction for positive genus:

  • Mathematically, this has not been considered before,
  • it is not quite the right generalization of subword complexes as defined by Knutson-Miller. In particular, such complexes do not need to be vertex-decomposable or shellable.
  • we have a proper generalization which will have similar combinatorics and will capture the needed behaviour for positive genus
  • our paper in which I introduce these things is still on hold and we will need at least another 3-6 months before getting this into a state to make it publicly available

I should be able to quickly look at your branch during the weekend to clean things up a bit. Or do you prefer to work things out yourself?

Cheers, Christian

comment:16 Changed 3 years ago by git

  • Commit changed from 76afdca9d2842a4f0a4060f60130bb37c41df99f to 88643156a58720df968a0cb95e38f94668871a1a

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

8864315trac #11010 one more doctest passing

comment:17 follow-up: Changed 3 years ago by chapoton

Hello Christian,

I would be happy to see you come back here. The code is not very easy to understand by me alone.

The remaining issues (15 doctests) seem to be related to a strange use of roots, where Weyl group elements seem to be supposed to act on the indices of roots in the list of roots. And they do not, of course.

I will not do anything more on the ticket until next week.

comment:18 in reply to: ↑ 17 Changed 3 years ago by stumpc5

Replying to chapoton:

Hi,

I would be happy to see you come back here. The code is not very easy to understand by me alone.

I spend some time today trying to get Sage working properly again, but I failed so far. I guess I need someone helping me to work properly with git...

I don't know how to resolve that now (and I also don't know if I tried to upgrade to the newest development version or the latest stable version).

The remaining issues (15 doctests) seem to be related to a strange use of roots, where Weyl group elements seem to be supposed to act on the indices of roots in the list of roots. And they do not, of course.

I use the CHEVIE implementation of finite reflection groups which are implemented as permutation groups on indices of roots. This speed things drastically, but it is not reall mathematically clean. Moreover, I just looked at the code, and

  • I also pushed many non-features that are there only for my research (and for which I cannot prove what I want to prove)
  • Many features only make sense for finite Coxeter groups, so such checks are still to be implemented.

comment:19 Changed 3 years ago by stumpc5

Hello Frederic,

I now spent some time playing with the patch.

My main concern is: I can re-implement things to work with any implementation of Coxeter groups (i.e., with the current implementations of Weyl groups and Coxeter groups). BUT: this will slow down things drastically!

So we either leave this ticket open and wait for my CHEVIE version of Coxeter groups, #11187 (I am not too positive that these will make it into Sage this year though), or we get rid of the speedy implementations and have a slow version to play in very low ranks that can be used soon.

Cheers, Christian

comment:20 Changed 3 years ago by stumpc5

  • Branch changed from u/chapoton/11010 to u/stumpc5/ticket/11010
  • Modified changed from 03/17/14 09:12:06 to 03/17/14 09:12:06

comment:21 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:22 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:23 Changed 22 months ago by git

  • Commit changed from 88643156a58720df968a0cb95e38f94668871a1a to ab914f50d616ad4df0fbd5a8c587d1843cd0a58e

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

014fec2got the doctests of complex reflection group category pass
d3198f9fixing a few bugs on coxeter number and fundamental invariants
e2736f6added another doctest
8db1deefirst version of adding functionality to coxeter group categories
425ca38first version of coxeter groups from chevie with all tests passing!
c0a036bAvoid creation of _reduced_word during iteration
405b26cturned well-generated into an axiom
0639665fixed a but with Word(reduced_word) we worked on yesterday
28f8abdadded a test for well generated
ab914f5first version of the subword complexes as of 2015

comment:24 Changed 22 months ago by vpilaud

  • Branch changed from u/stumpc5/ticket/11010 to u/vpilaud/ticket/11010

comment:25 Changed 22 months ago by git

  • Commit changed from ab914f50d616ad4df0fbd5a8c587d1843cd0a58e to bd4f2ada52cbe683bd8c768e82f729a24b73e8aa

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

bd4f2adimprove the subword complex, first version of plot in type B

comment:26 Changed 22 months ago by stumpc5

Vincent, please remove the c and the html file again from the branch since they are not supposed to be going into sage.

comment:27 Changed 22 months ago by git

  • Commit changed from bd4f2ada52cbe683bd8c768e82f729a24b73e8aa to f165d54668bc6342df43c2c31877195aa2f7765d

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

f165d54pseudoline representation of facets of type B subword complexes, more documentation

comment:28 Changed 22 months ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.8

does not apply, needs to be rebased

comment:29 Changed 22 months ago by chapoton

  • Dependencies changed from #12774 to #12774, #11187

comment:30 Changed 17 months ago by chapoton

  • Branch changed from u/vpilaud/ticket/11010 to u/chapoton/11010
  • Commit changed from f165d54668bc6342df43c2c31877195aa2f7765d to 0aa6989fc49f62d72c8d6452f4bb46b4533518d7

I have made a new branch, by disentangling u/vpilaud/ticket/11010 from #11187 (but sadly still depending on #11187).

It seems that all my previous work on this ticket was lost. I do not appreciate.


New commits:

bbff606first version of the subword complexes as of 2015
0aa6989trac #11010 full pep8 and pyflakes

comment:31 Changed 17 months ago by stumpc5

It seems that all my previous work on this ticket was lost. I do not appreciate.

I don't understand. How can work be lost on a ticket?

comment:32 Changed 17 months ago by chapoton

Sorry for having been a little rude. I was tired and angry yesterday evening.

It remains that I have done twice the same clean-up work. I think it was because the branch by Vincent was not based on my previous branch.

I would really like if this ticket would no longer depend on #11187, which has no hope to enter sage as long as it depends on gap3.

You said once that it could be possible, but with a high speed penalty. Maybe this is nevertheless the way to go.

comment:33 Changed 17 months ago by git

  • Commit changed from 0aa6989fc49f62d72c8d6452f4bb46b4533518d7 to 32da9d29df4394dc0553d760bd2482f80dc07fed

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

9dccaa7Merge branch 'u/chapoton/11010' into 6.10.b4
32da9d2trac #11010 newstyle doctest continuation

comment:34 Changed 17 months ago by git

  • Commit changed from 32da9d29df4394dc0553d760bd2482f80dc07fed to 046879866540893d9d7f678c2962728a322f0ebd

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

0468798trac #11010 make sure that doc builds

comment:35 Changed 17 months ago by chapoton

  • Milestone changed from sage-6.8 to sage-6.10

comment:36 Changed 17 months ago by tscrim

As far as I can see, just one thing is needed:

  • reflections() (technically nr_reflections(), but we can just do len(self.reflections()))

We can construct this easily enough by conjugation of simple reflections:

sage: W = CoxeterGroup(['A',2])
sage: S = W.simple_reflections()
sage: I = W.index_set()
sage: R = RecursivelyEnumeratedSet(S, lambda x: [S[i]*x*S[i] for i in I if not x.has_descent(i)], structure="graded")
sage: list(R)
[
[-1  1]  [ 1  0]  [ 0 -1]
[ 0  1], [ 1 -1], [-1  0]
]
sage: all(x.absolute_length() for x in R)
True

The apply_simple_reflection is already a part of Sage for Coxeter groups.

Aside - This probably deserves to be in the category of finite Coxeter groups, but there is some slight conflict with this

sage: W = WeylGroup(['A',2], prefix='s')
sage: W.reflections()
Finite family {s1*s2*s1: (1, 0, -1), s1: (1, -1, 0), s2: (0, 1, -1)}

(Trying to do this on the Weyl group on the root system seems to run forever and ctrl-C out results in a nasty error message.)

comment:37 Changed 17 months ago by git

  • Commit changed from 046879866540893d9d7f678c2962728a322f0ebd to 2d84a4b1f6bdb6e75c0aa1b1a571620c737e0cb6

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

2d84a4btrac #11010 little more doc

comment:38 Changed 16 months ago by git

  • Commit changed from 2d84a4b1f6bdb6e75c0aa1b1a571620c737e0cb6 to 3f104918266a45d31cf272fdc03786e302e98c80

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

518eb3cMerge branch 'u/chapoton/11010' into 6.10.rc0
3f10491trac #11010 more doc

comment:39 Changed 16 months ago by git

  • Commit changed from 3f104918266a45d31cf272fdc03786e302e98c80 to cf864bd924979485335feb4521a9c6b166eb36aa

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

cf864bdtrac #11010 trying to add more doc (but alas blindly)

comment:40 Changed 16 months ago by git

  • Commit changed from cf864bd924979485335feb4521a9c6b166eb36aa to 1447f4bbe8aed1adcabccf762dce56b7b828a3cf

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

1447f4btrac #11010 a lot of work, still some parts are broken

comment:41 Changed 16 months ago by git

  • Commit changed from 1447f4bbe8aed1adcabccf762dce56b7b828a3cf to 004f8a8355bbd341aa6140d7d6e66d5d0ed81bc9

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

004f8a8trac #11010 caching the positive roots

comment:42 Changed 16 months ago by git

  • Commit changed from 004f8a8355bbd341aa6140d7d6e66d5d0ed81bc9 to 8b5db7a55c65b449c00c18be8e6c0cc2ddaff199

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

8b5db7atrac 11010 more caching

comment:43 Changed 16 months ago by git

  • Commit changed from 8b5db7a55c65b449c00c18be8e6c0cc2ddaff199 to 4e50eec043dd0035b96f48489fababf928f9f21a

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

4e50eectrac #11010 more work on doc and code of subword complexes

comment:44 Changed 16 months ago by git

  • Commit changed from 4e50eec043dd0035b96f48489fababf928f9f21a to 567a7670bc8fdfe29367079ed8fcb3be4576fbb2

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

567a767trac #11010 almost working now (but plot is broken)

comment:45 Changed 16 months ago by chapoton

Ok, guys. We are almost there. Most of the things are working and hopefully not too slow. I have worked hard for two days, so please react !

Just 2 small remaining problems (unless the bot reveals some others):

  • the "plot" code is broken, maybe we can remove that for the moment ? unless somebody wants to repair it ?
  • the "root_polytope" method is not documented, any objection to its removal ? unless you can provide a working example ?

cheers,

Frederic

Last edited 16 months ago by chapoton (previous) (diff)

comment:46 Changed 16 months ago by chapoton

  • Dependencies #12774, #11187 deleted

comment:47 Changed 16 months ago by chapoton

  • Work issues changed from coverage to plot and root_polytope

comment:48 Changed 16 months ago by stumpc5

Hi Frederic -- thanks a lot for all your work here! I am still too busy to contribute, but I can promise to have had a closer look by the end of the week, together with my opinion on how to proceed and a few speed tests... Cheers, Christian

comment:49 Changed 16 months ago by git

  • Commit changed from 567a7670bc8fdfe29367079ed8fcb3be4576fbb2 to d0a5f71b2d4b26e7cb5a932181e1390d45072777

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

d0a5f71trac #11010 some details

comment:50 Changed 16 months ago by stumpc5

Okay, things seem to work, great!

  • Root polytope: all this patch was basically parked on trac for my own research. And this method is stuff for which nothing is proven so far, so it should not be part of the ticket anymore.
  • Plot: what do you have in mind outside of types A and B?
  • Here are a few timings:

This patch:

sage: W = WeylGroup(['A',6])
sage: c = list(W.index_set()); Q = c + W.w0.coxeter_sorting_word(c)
sage: %time S = SubwordComplex(Q,W.w0)
CPU times: user 23.6 s, sys: 36 ms, total: 23.6 s
Wall time: 23.6 s
sage: len(S)
429

The same patch based on the Coxeter group implementation:

sage: W = CoxeterGroup(['A',6])                                    
sage: c = list(W.index_set()); Q = c + W.w0.coxeter_sorting_word(c)
sage: %time S = SubwordComplex(Q,W.w0)                             
CPU times: user 0.41 s, sys: 0.00 s, total: 0.41 s
Wall time: 0.41 s
sage: len(S)                                                       
429

comment:51 Changed 16 months ago by stumpc5

  • Branch changed from u/chapoton/11010 to u/stumpc5/11010

comment:52 Changed 16 months ago by stumpc5

  • Commit changed from d0a5f71b2d4b26e7cb5a932181e1390d45072777 to 6b65d7003f98adc0aab9984cfec28de9b6311eab
  • Status changed from needs_work to needs_review
  • Work issues plot and root_polytope deleted

I removed some experimental code that is not supposed to be merged (this includes the root polytope), and fixed the plot. This file's tests pass locally, so let's see what the patchbot says...


New commits:

5c93cd4Merge branch 'u/chapoton/11010' of git://trac.sagemath.org/sage into 11010-new
6b65d70removed a few experimental methods, fixed the plot

comment:53 Changed 16 months ago by chapoton

I think one should put a large Warning about

To be used only with Coxeter groups in the implementation='reflection'

In fact, it is not only slow but mostly broken with Weyl groups.

Side remark : it seems that one cannot use the quadratic fields Q(sqrt2) for type B and Q(sqrt5) for type H. To be fixed in another ticket, maybe.

comment:54 Changed 16 months ago by stumpc5

In fact, it is not only slow but mostly broken with Weyl groups.

Oh yes, the doctests were written for implementation='reflection' so it does not show that it is broken for Weyl groups.

I will have a quick look whether I can fix that!

comment:55 Changed 16 months ago by tscrim

I'm not too surprised WeylGroup is slower; it goes through the GAP interface (in particular, for multiplication) whereas the reflection implementation (of Coxeter groups) does not. However because of this, it can't get things like conjugacy classes. There is probably quite a bit we can improve with (GAP) matrix groups, but that is an issue for another ticket.

However, it surprises me a little bit that it is broken for Weyl groups because the reflection implementation was fairly minimal (up to what comes from the category).

comment:56 follow-up: Changed 16 months ago by stumpc5

First, the definition of subword complexes is for Coxeter groups (finite or infinite), but many of its properties (see my paper with Vincent referenced in the header) come from understanding roots and weights. (This means that the construction works, but many methods don't.)

Second, I think the current failure is only because the method WeylGroup.roots is missing, as is the method WeylGroupElement.action_on_root_indices.

  • Do you know how I can get the list of (simple, positive, almost positive, all) roots of a root system when I only have the Weyl group W at hand? It seems that I have to build a root system from the Cartan type, though it seems more natural to have a method WeylGroup.root_system or at least WeylGroup.cartan_matrix, wouldn't it?
Last edited 16 months ago by stumpc5 (previous) (diff)

comment:57 follow-up: Changed 16 months ago by chapoton

  • Branch changed from u/stumpc5/11010 to u/chapoton/11010
  • Commit changed from 6b65d7003f98adc0aab9984cfec28de9b6311eab to f4bc87fa3a884cfb45e9c90bd37c4d1f55c8d090

New branch, where I have added a warning.

Do you really want this to work for Weyl groups ?

You can build roots for Weyl groups just as I did for the Coxeter groups, using a reduced word for w0. But it will not be enough..


New commits:

59e9e2dMerge branch 'u/stumpc5/11010' into 6.10
f4bc87ftrac #11010 some doc details, and pep8

comment:58 in reply to: ↑ 57 Changed 16 months ago by stumpc5

Replying to chapoton:

Do you really want this to work for Weyl groups ?

I just want to quickly see if I can get the method root_configuration working since many others only depend on this one. And many people do use Weyl groups...

comment:59 in reply to: ↑ 56 Changed 16 months ago by stumpc5

Replying to stumpc5:

  • Do you know how I can get the list of (simple, positive, almost positive, all) roots of a root system when I only have the Weyl group W at hand? It seems that I have to build a root system from the Cartan type, though it seems more natural to have a method WeylGroup.root_system or at least WeylGroup.cartan_matrix, wouldn't it?

Ah, WeylGroup.domain does it...

comment:60 Changed 16 months ago by git

  • Commit changed from f4bc87fa3a884cfb45e9c90bd37c4d1f55c8d090 to e252db3ee11ba3642fdcfdad8c472e7e0602f2b6

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

e252db3trac #11010 bad unicode character removed

comment:61 Changed 16 months ago by stumpc5

Do you really want this to work for Weyl groups ?

Okay, I give up on this -- I tried the past hours to get it done for Weyl groups, but I basically have to fix every single method, and even have to implement my own fundamental weights in order to make this work for Weyl groups. If this is crucial at some point, we can still get back to this...

comment:62 Changed 15 months ago by stumpc5

The patchbot is happy, so I think this is ready to go -- or do you see any remaining problems?

comment:63 Changed 15 months ago by git

  • Commit changed from e252db3ee11ba3642fdcfdad8c472e7e0602f2b6 to 8b79780943df18379d9e16ce016abf83662ca261

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

7d9f5e8Merge branch 'u/chapoton/11010' into 7.10.b2
8b79780trac #11010 one typo

comment:64 follow-up: Changed 15 months ago by chapoton

I am not quite satisfied with the custom naive type A/B recognition in the plot method. Travis, is there a better way ? or even a way to have the correct type printed in the repr ?

comment:65 in reply to: ↑ 64 Changed 15 months ago by tscrim

Replying to chapoton:

I am not quite satisfied with the custom naive type A/B recognition in the plot method. Travis, is there a better way ? or even a way to have the correct type printed in the repr ?

You can use the (relatively recent) Coxeter matrices and types:

sage: W = CoxeterGroup(['C',4])
sage: T = W.coxeter_matrix().coxeter_type(); T
Coxeter type of ['C', 4]

The current implementation for Coxeter types essentially wraps the Cartan types, so there is a "different" type for B and C. (This was done because there was a lot of information that could be pulled from the Cartan type, as there is a lot of hard-coded data, and I didn't want to have to try and decide on a naming scheme. I fully take the blame for the hacks involved.)

However, the information you're after is inside the wrapped type:

sage: T.cartan_type()
['B', 4]
sage: T.cartan_type().type()
'B'
sage: T.cartan_type().rank()
4

comment:66 Changed 15 months ago by git

  • Commit changed from 8b79780943df18379d9e16ce016abf83662ca261 to 1b10c95a312057dee7d920ff4d06ea713ef5338b

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

1b10c95trac #11010 re-introducing Cartan types

comment:67 Changed 15 months ago by chapoton

ok, thanks Travis. I have done the needed changes, I think.

comment:68 Changed 15 months ago by chapoton

  • Milestone changed from sage-6.10 to sage-7.0
  • Reviewers set to Frédéric Chapoton

ok, I think that this can go. Christian, if you agree, set to positive.

comment:69 Changed 15 months ago by stumpc5

  • Status changed from needs_review to positive_review

comment:70 Changed 15 months ago by tscrim

Does the plotting also work for type C, being that it is the dual of type B (and we are dealing with Cartan types)?

comment:71 Changed 15 months ago by stumpc5

  • Status changed from positive_review to needs_work

I am rather confused now:

  • First Sage start
    sage: W = CoxeterGroup(['B',4])
    sage: W.coxeter_matrix().coxeter_type()
    Coxeter type of ['B', 4]
    sage: W = CoxeterGroup(['C',4])
    sage: W.coxeter_matrix().coxeter_type()
    Coxeter type of ['B', 4]
    
  • Second Sage start
    sage: W = CoxeterGroup(['C',4])
    sage: W.coxeter_matrix().coxeter_type()
    Coxeter type of ['C', 4]
    sage: W = CoxeterGroup(['B',4])
    sage: W.coxeter_matrix().coxeter_type()
    Coxeter type of ['C', 4]
    

In particular, the root systems W.roots() are not correct in the two second calls of CoxeterGroup.

Also, the plotting now assumes that the first index is the type B/C special one. This is, it is expected that

sage: s0 = W.simple_reflection(W.index-set()[0])
sage: s1 = W.simple_reflection(W.index-set()[1])
sage: (s0*s1).order()
4

I doubt that this is correct in the current implementation of Coxeter groups where the last index is expected to be special. If so, I will add some magic to figure out the proper labelling...

Last edited 15 months ago by stumpc5 (previous) (diff)

comment:72 Changed 15 months ago by tscrim

The first issue is the result of my quick hacking to get something working. The Coxeter matrices are equal for the Cartan types B and C, and the Coxeter matrix is the information passed to the UniqueRepresentation cache. There are a number of ways to go moving forward, but I think that deserves its own separate ticket.

For the second issue, yes, you are correct. The n-1 and n nodes have the order 4 relation between them, as this agrees with the corresponding B/C Cartan types (and the Coxeter matrix). You are welcome to relabel the Coxeter type, which should work, or go to the Coxeter graph and find the edge label4ed by 4 and use those nodes. Personally, I would use the latter option since it is independent of the labeling.

comment:73 Changed 15 months ago by stumpc5

I also have the problem that the plot only allows irreducible types. When playing with reducible types, I got to the types B vs. G bug in #19830...

comment:74 Changed 15 months ago by stumpc5

Last edited 15 months ago by stumpc5 (previous) (diff)

comment:75 Changed 15 months ago by stumpc5

  • Branch changed from u/chapoton/11010 to u/stumpc5/11010

comment:76 Changed 15 months ago by chapoton

  • Commit changed from 1b10c95a312057dee7d920ff4d06ea713ef5338b to 839b2e2437ae30bb4bd42095f8f112b9dbe50eee

Is this "needs_review" ?


New commits:

4864a64Merge branch '11010-new' into u/stumpc5/11010-new
7722489Merge branch 'u/chapoton/11010' of git://trac.sagemath.org/sage into u/stumpc5/11010-new
839b2e2fixing the index set for the plot

comment:77 Changed 15 months ago by stumpc5

Is this "needs_review" ?

Not quite, I wanted to add a few more doctests for the type B plotting with various indexing sets. But then saw that the indexing in CoxeterGroup is corrupted, see #19830. This is another issue, but I still want to add more examples/testing.

comment:78 Changed 15 months ago by git

  • Commit changed from 839b2e2437ae30bb4bd42095f8f112b9dbe50eee to 0f90c266d0b653830d4555b839b041d633e3935d

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

0f90c26fixed another b/c bug + some cosmetics to the code

comment:79 Changed 15 months ago by git

  • Commit changed from 0f90c266d0b653830d4555b839b041d633e3935d to e8e160c5cbca21a348b6505414b7952e073fd56d

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

e8e160cfixed a docstring and adding one more test

comment:80 Changed 15 months ago by stumpc5

  • Status changed from needs_work to needs_review

comment:81 Changed 15 months ago by chapoton

  • Branch changed from u/stumpc5/11010 to u/chapoton/11010
  • Commit changed from e8e160c5cbca21a348b6505414b7952e073fd56d to 17518c11f06b77c1823a5b0950285085540a41b2
  • Status changed from needs_review to positive_review

ok, good enough for me.

I have made two minor doc changes, and now set this again to positive review.

Any further changes will have to be in another ticket.


New commits:

17518c1trac #11010 two details

comment:82 Changed 15 months ago by vbraun

  • Branch changed from u/chapoton/11010 to 17518c11f06b77c1823a5b0950285085540a41b2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.