#11187 closed enhancement (fixed)
Implementation of finite reflection groups
Reported by:  Christian Stump  Owned by:  tbd 

Priority:  major  Milestone:  sage7.2 
Component:  combinatorics  Keywords:  reflection group, days49, days64.5, days80 
Cc:  Vivien Ripoll, Kevin Dilks, Simon King, Jean Michel, Florent Hivert  Merged in:  
Authors:  Christian Stump, Frédéric Chapoton, Nicolas M. Thiéry, Travis Scrimshaw  Reviewers:  Christian Stump, Frédéric Chapoton, Nicolas M. Thiéry, Vivien Ripoll, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  0b6d895 (Commits, GitHub, GitLab)  Commit:  0b6d895c865ebdb319de6d58e862e7ae1a1cb784 
Dependencies:  Stopgaps: 
Description (last modified by )
This patch implements reflection groups to work with finite (crystallographic, real, complex) reflection groups. It is based on the GAP3 package chevie which is now an experimental package, see #20107.
sage: W = ReflectionGroup(['A',2],5,23); W Reducible complex reflection group of rank 7 and type A2 x ST5 x H3 sage: W.irreducible_components() [Irreducible real reflection group of rank 2 and type A2, Irreducible complex reflection group of rank 2 and type ST5, Irreducible real reflection group of rank 3 and type H3]
Attachments (1)
Change History (279)
comment:1 Changed 12 years ago by
Component:  PLEASE CHANGE → combinatorics 

Status:  new → needs_work 
comment:2 Changed 12 years ago by
comment:3 Changed 12 years ago by
Status:  needs_work → needs_review 

Apply only trac_11187finite_reflection_groupscs.patch
comment:4 Changed 12 years ago by
Dependencies:  → 8327 

Description:  modified (diff) 
comment:5 Changed 12 years ago by
Status:  needs_review → needs_work 

comment:7 Changed 12 years ago by
Dependencies:  8327 → #8327 

comment:8 Changed 12 years ago by
Milestone:  → sage4.7.2 

Changed 10 years ago by
Attachment:  trac_11187finite_reflection_groupscs.patch added 

comment:9 Changed 10 years ago by
+ class SubcategoryMethods: + + @cached_method + def Irreducible(self): + """ + Returns the full subcategory of the irreducible objects of ``self``. + + EXAMPLES:: + + sage: CoxeterGroups().Irreducible() + Category of irreducible coxeter groups + + TESTS:: + + sage: AffineWeylGroups().Irreducible.__module__ + 'sage.categories.complex_reflection_groups' + """ + return self._with_adjectives(('Irreducible',)) + + class Irreducible(AdjectiveCategory): + pass +
comment:10 Changed 10 years ago by
Dependencies:  #8327 → #8327 #14137 

I've factored out the CartanMatrix
class into #14137.
comment:11 followup: 12 Changed 10 years ago by
Keywords:  days49 added 

comment:12 Changed 10 years ago by
Replying to tscrim:
I've rebased the patch and noticed that this will likely depend on #10963. I'll post a new version once I completely untangle what is going on with #10963.
I always made some minor changes on this patch locally  I would actually only start working on this patch after you guys are finished with #10963. If you want to actively work on this patch, ping me again, and we can discuss what is still needed to finilize this patch.
Cheers, Christian
comment:13 Changed 9 years ago by
Milestone:  sage5.11 → sage5.12 

comment:14 Changed 9 years ago by
Milestone:  sage6.1 → sage6.2 

comment:15 Changed 9 years ago by
Milestone:  sage6.2 → sage6.3 

comment:16 followup: 17 Changed 9 years ago by
Hello Christian
Do you have a git branch for this ticket somewhere ? or should I try to resurrect the patch ?
comment:17 Changed 9 years ago by
Do you have a git branch for this ticket somewhere ? or should I try to resurrect the patch ?
No, I don't. But I have a local old version of Sage containing that code and quite a bit more, so I wonder if I should/will try to organize it a bit and put it up here.
Do you need any from that code in particular (in which case I am more tempted to work on that), or is that just a general question?
comment:18 Changed 9 years ago by
Hello, no, no real need for this code, just asking a general question. Now that #10963 is over.
comment:19 Changed 9 years ago by
Branch:  → public/groups/finite_reflection_groups11187 

Commit:  → 1f3fe1e0caa28a1cb6e2d17a61d94c89460e2bd4 
Here's a git version of this patch with changes necessary to get Sage to start (I did not run any tests, so I am not saying things are working). There might be other changes to the files in question which need to be moved over too. Just as a general note from doing the rebasing, there are a bunch of methods missing doctests.
New commits:
3489e1c  #11187 Implementation of finite reflection groups using the GAP3 package chevie

1f3fe1e  Some other fixes from changes to Sage in order to get Sage to start.

comment:20 Changed 9 years ago by
Commit:  1f3fe1e0caa28a1cb6e2d17a61d94c89460e2bd4 → b60e61b6695fb1cde48d867390d50c32569fc2b0 

comment:21 Changed 9 years ago by
Commit:  b60e61b6695fb1cde48d867390d50c32569fc2b0 → 1de360ff9e9c4db57e7de8b4ea45bbc4807e243c 

Branch pushed to git repo; I updated commit sha1. New commits:
1de360f  trac #11187 crystallo takes 2 l

comment:22 Changed 9 years ago by
Commit:  1de360ff9e9c4db57e7de8b4ea45bbc4807e243c → deb13e3c50d259c1708aaa815300d94045a14b07 

Branch pushed to git repo; I updated commit sha1. New commits:
deb13e3  trac #11187 trying to make doctest pass

comment:23 Changed 9 years ago by
Commit:  deb13e3c50d259c1708aaa815300d94045a14b07 → b6c1a6273f96f1b5d6103b3090eee3633b2f54b2 

Branch pushed to git repo; I updated commit sha1. New commits:
b6c1a62  trac #11187 a few more corrections

comment:24 Changed 9 years ago by
Commit:  b6c1a6273f96f1b5d6103b3090eee3633b2f54b2 → becb1cab8eacec6e60661677fb2c1fcfe2429afe 

Branch pushed to git repo; I updated commit sha1. New commits:
becb1ca  trac #11187 a few more changes, removing duplicate functions

comment:25 Changed 9 years ago by
Commit:  becb1cab8eacec6e60661677fb2c1fcfe2429afe → e97998c532abc75eec6b18fecff6769601a1bade 

comment:26 Changed 9 years ago by
Commit:  e97998c532abc75eec6b18fecff6769601a1bade → 03111c8373909a6f4ac8e7c8b63625c74b4077a5 

Branch pushed to git repo; I updated commit sha1. New commits:
03111c8  trac #11187 a little pyflakes cleanup

comment:27 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:28 Changed 8 years ago by
Commit:  03111c8373909a6f4ac8e7c8b63625c74b4077a5 → 4a37eac384b296acde87af5bddbe39a4ba8addf0 

comment:29 Changed 8 years ago by
Commit:  4a37eac384b296acde87af5bddbe39a4ba8addf0 → 527cb502a4f3729472344a7fbb894502ddd03050 

Branch pushed to git repo; I updated commit sha1. New commits:
527cb50  Merge branch 'public/groups/finite_reflection_groups11187' into 6.6.b1

comment:30 Changed 8 years ago by
Cc:  Vivien Ripoll added 

comment:31 Changed 8 years ago by
Commit:  527cb502a4f3729472344a7fbb894502ddd03050 → f2fbe0ad1c694936e6d15195055c261aadc17e1d 

Branch pushed to git repo; I updated commit sha1. New commits:
f2fbe0a  merged in 6.8.beta2

comment:32 Changed 8 years ago by
Branch:  public/groups/finite_reflection_groups11187 → u/stumpc5/11187 

comment:33 Changed 8 years ago by
Commit:  f2fbe0ad1c694936e6d15195055c261aadc17e1d → ba7b5f092f41da9bbdb18455ecc5af2623dc42cc 

Branch pushed to git repo; I updated commit sha1. New commits:
ba7b5f0  made Sage not crash on startup

comment:34 Changed 8 years ago by
Cc:  Kevin Dilks added 

comment:35 Changed 8 years ago by
Branch:  u/stumpc5/11187 → u/stumpc5/11187new 

comment:36 Changed 8 years ago by
Commit:  ba7b5f092f41da9bbdb18455ecc5af2623dc42cc → 6ca40fa7459d11bf9c3564777d962a7493eb476c 

comment:37 Changed 8 years ago by
Commit:  6ca40fa7459d11bf9c3564777d962a7493eb476c → 33f93c6061d90dd3849525f63aa7ffaa21049492 

Branch pushed to git repo; I updated commit sha1. New commits:
33f93c6  removed the magma iterator

comment:38 Changed 8 years ago by
Commit:  33f93c6061d90dd3849525f63aa7ffaa21049492 → 08a782138c922172d901834b691a80ec850f9bb2 

Branch pushed to git repo; I updated commit sha1. New commits:
08a7821  fixed all doctests in src/sage/combinat/root_system/complex_reflection_group.py

comment:39 Changed 8 years ago by
Commit:  08a782138c922172d901834b691a80ec850f9bb2 → 014fec229c73bb16da7825d24633c47076f22c4c 

Branch pushed to git repo; I updated commit sha1. New commits:
014fec2  got the doctests of complex reflection group category pass

comment:40 Changed 8 years ago by
Commit:  014fec229c73bb16da7825d24633c47076f22c4c → d3198f92cb72fd0ca67c4118ddcb00289143bd62 

Branch pushed to git repo; I updated commit sha1. New commits:
d3198f9  fixing a few bugs on coxeter number and fundamental invariants

comment:41 Changed 8 years ago by
Commit:  d3198f92cb72fd0ca67c4118ddcb00289143bd62 → 8db1deeed4893580fd7c88d42d2fb9366a895bcf 

comment:42 Changed 8 years ago by
Commit:  8db1deeed4893580fd7c88d42d2fb9366a895bcf → 425ca38c084a51fa56c301153165269031fb5e0d 

Branch pushed to git repo; I updated commit sha1. New commits:
425ca38  first version of coxeter groups from chevie with all tests passing!

comment:43 Changed 8 years ago by
Cc:  Simon King added 

comment:44 Changed 8 years ago by
The current code gives
sage: C = ComplexReflectionGroups() sage: C.Irreducible() Category of complex reflection groups sage: C.Irreducible() is C True
Is that what you intended? Perhaps the "irreducible" axiom should be defined on ComplexReflectionGroups().Finite()
, not on ComplexReflectionGroups()
.
comment:45 Changed 8 years ago by
To summarise the discussion Christian and I just had: I believe WellGenerated
should be an axiom. Christian said that the general notion of wellgenerated is a bit tricky for infinite groups. Hence, he actually only wants to take into account wellgeneratedness for finite complex reflection groups.
Hence, the aim would be to define WellGeneratedComplexReflectionGroups
as application of an axiom to ComplexReflectionGroups.Finite
. However, I believe that the name should contain the word "finite" if we really want to focus on finite wellgenerated groups. Alternatively, it should be implemented as a nestednested class WellGenerated
within the nested class ComplexReflectionGroups.Finite
.
I also think that WellGeneratedComplexReflectionGroups
should provide a parent method _test_well_generated
that tests whether self
is wellgenerated. Such method would automatically be executed when running the test suite.
comment:46 Changed 8 years ago by
We have discussed already that in order to fix the Irreducible
axiom, it is needed (in addition to the nested class) to have a SubcategoryMethod
called Irreducible
.
Note that if we define WellGeneratedComplexReflectionGroups
as a subcategory of ComplexReflectionGroups.Finite()
, we should probably remove its subcategorymethods Finite
and Irreducible
, since both axioms have already been defined in the supercategory.
comment:47 Changed 8 years ago by
The iterators currently cache the list of all elements of the group. Is that (always) a good idea?
comment:48 Changed 8 years ago by
Branch:  u/stumpc5/11187new → u/SimonKing/11187new 

comment:49 Changed 8 years ago by
Commit:  425ca38c084a51fa56c301153165269031fb5e0d → c0a036b2e2f01b1be6a7d004373fafe0c399e14d 

After discussion with Christian, the iterator of a complex reflection group is a lot faster. What we did was to keep ._reduced_word
as a list, and only transform it into an actual word when needed (i.e., in the .reduced_word()
method).
Before:
sage: G = ComplexReflectionGroup([2,1,4]) sage: %timeit L = list(G); G._elements=None The slowest run took 6.02 times longer than the fastest. This could mean that an intermediate result is being cached 1 loops, best of 3: 66.4 ms per loop
After:
sage: G = ComplexReflectionGroup([2,1,4]) sage: %timeit L = list(G); G._elements=None The slowest run took 66.44 times longer than the fastest. This could mean that an intermediate result is being cached 1 loops, best of 3: 5.56 ms per loop
New commits:
c0a036b  Avoid creation of _reduced_word during iteration

comment:50 Changed 8 years ago by
Branch:  u/SimonKing/11187new → u/stumpc5/11187new 

comment:51 Changed 8 years ago by
Commit:  c0a036b2e2f01b1be6a7d004373fafe0c399e14d → 0639665e4f6ac2bee4a4cc9dfcaed5963d910155 

Milestone:  sage6.4 → sage6.8 
comment:52 Changed 8 years ago by
Commit:  0639665e4f6ac2bee4a4cc9dfcaed5963d910155 → 28f8abd2eb45700978592f1781a1f87e3c4d5e69 

Branch pushed to git repo; I updated commit sha1. New commits:
28f8abd  added a test for well generated

comment:53 Changed 8 years ago by
Hi Christian,
This is not how the tests work:
def _test_well_generated(self, **options): tester = self._tester(**options) return self.is_well_generated()
Instead, do
def _test_well_generated(self, **options): tester = self._tester(**options) tester.assert_(self.is_well_generated())
or something similar.
comment:54 Changed 8 years ago by
see patchbot reports :
 some doctests should be marked with
# optional  chevie
or whatever  doctest continuation is now
....:
and not...
 raise statements should use py3 syntax
raise Error('message')
comment:55 Changed 8 years ago by
Commit:  28f8abd2eb45700978592f1781a1f87e3c4d5e69 → 697cd3f3077ef749bee642e0281b24fc66c3cbeb 

comment:56 followup: 58 Changed 8 years ago by
@Simon and Nicolas: The category for complex reflection groups (with axioms finite/irreducible/wellgenerated) is now sort of ready. I would now like to make ComplexReflectionGroups().Finite().WellGenerated()
a super category of FiniteCoxeterGroups
.
 Is that what I should do?
 If I simply add a method
supercategories
toFiniteCoxeterGroups
, its string repr changes in a not desired way. How should I treat that?
Thanks, Christian
comment:57 Changed 8 years ago by
Commit:  697cd3f3077ef749bee642e0281b24fc66c3cbeb → 92ab198593eb2d772c48c4cfb80408d6da1daaa9 

Branch pushed to git repo; I updated commit sha1. New commits:
92ab198  made the category of coxeter groups a subcategory of complex reflection groups

comment:58 Changed 8 years ago by
Replying to stumpc5:
@Simon and Nicolas: The category for complex reflection groups (with axioms finite/irreducible/wellgenerated) is now sort of ready. I would now like to make
ComplexReflectionGroups().Finite().WellGenerated()
a super category ofFiniteCoxeterGroups
.
 Is that what I should do?
 If I simply add a method
supercategories
toFiniteCoxeterGroups
, its string repr changes in a not desired way. How should I treat that?
It sounds more like you want to add ComplexReflectionGroups().WellGenerated()
as a supercategory to CoxeterGroups
, in particular to replace Groups().FinitelyGenerated()
by ComplexReflectionGroups().WellGenerated()
. I also think ComplexReflectionGroups()
should be a subcategory of Groups().FinitelyGenerated()
(otherwise the additional axiom should go on the supercategory for CoxeterGroups()
). The finite part will take care of itself by the CategoryWithAxiom
magic.
comment:59 Changed 8 years ago by
Commit:  92ab198593eb2d772c48c4cfb80408d6da1daaa9 → abc690c06e9f6b60bde44f689259d3787d5681ea 

Branch pushed to git repo; I updated commit sha1. New commits:
ec94ae3  added a few more doctests for CoxeterGroupsChevie

1878996  work in progress: new implementations are presumably correct, but existing bug discovered

5716df5  Fixed according to Travis's suggestions

4404133  Fixed according to Travis's suggestions, and now actually running I think

8fe1bb8  Tab fixed

a6dcb8b  merged with nowresolved conflicts

c37a5ef  things appear to work in the sense that all tests have passed.

1862325  removed trailing whitespace.

1f29ae6  reordered documentation, as requested by chapoton

abc690c  many doctests, and merged #18597

comment:60 followup: 62 Changed 8 years ago by
Hello,
 once again: doctest continuation is now
....:
and not...
 could you please put a positive review on the duplicate #9288 ?
comment:61 Changed 8 years ago by
Commit:  abc690c06e9f6b60bde44f689259d3787d5681ea → 742c0df5916cdd39d843553858f295df9a1ed18e 

Branch pushed to git repo; I updated commit sha1. New commits:
742c0df  replaced '... ' by '....:'

comment:62 Changed 8 years ago by
Hi
 once again: doctest continuation is now
....:
and not...
done
 could you please put a positive review on the duplicate #9288 ?
done
Thanks for looking at this (and all the other tickets!
Christian
comment:63 Changed 8 years ago by
Commit:  742c0df5916cdd39d843553858f295df9a1ed18e → df37b7e9dbec9913ad652826e165e7f534ded0d3 

Branch pushed to git repo; I updated commit sha1. New commits:
ab914f5  first version of the subword complexes as of 2015

cb3ca9c  improve the subword complex, in particular its documentation

f105602  Merge branch 'u/stumpc5/11010' into u/stumpc5/11187new

8618845  playing, I hopefully can reorganize this again

ab4999f  more intelligent fix a la stump

df37b7e  Merge branch 't/18610/bug__circular_descent_check_in_weylgroups' into u/stumpc5/11187new

comment:64 Changed 8 years ago by
Dependencies:  #8327 #14137 → #8327 #14137 #18590 

comment:65 Changed 8 years ago by
Dependencies:  #8327 #14137 #18590 → #8327 #14137 #18590 #18597 

comment:66 Changed 8 years ago by
Commit:  df37b7e9dbec9913ad652826e165e7f534ded0d3 → e180c3133fb8c90f941b3f6f4be8a10e0bb15065 

Branch pushed to git repo; I updated commit sha1. New commits:
e180c31  Revert "improve the subword complex, in particular its documentation"

comment:67 Changed 8 years ago by
Commit:  e180c3133fb8c90f941b3f6f4be8a10e0bb15065 → 450dadd370f2a8b1e6d05630d669742df3a1beef 

Branch pushed to git repo; I updated commit sha1. New commits:
450dadd  Revert "first version of the subword complexes as of 2015"

comment:68 Changed 8 years ago by
I took out the subword complex stuff which is handled in #11010, and which is depending on this one.
comment:69 Changed 8 years ago by
Commit:  450dadd370f2a8b1e6d05630d669742df3a1beef → e8e0d9ae9e5770b2088d6fb9ee6db412d21353a2 

Branch pushed to git repo; I updated commit sha1. New commits:
e8e0d9a  added two doctests

comment:70 Changed 8 years ago by
Dependencies:  #8327 #14137 #18590 #18597 → #8327 #14137 #18590 #18597 #18620 

comment:71 Changed 8 years ago by
Dependencies:  #8327 #14137 #18590 #18597 #18620 → #18620 

comment:72 Changed 8 years ago by
Commit:  e8e0d9ae9e5770b2088d6fb9ee6db412d21353a2 → 80b9a54c6f35f6204fd226df8e95df45b05435d3 

comment:73 Changed 8 years ago by
Commit:  80b9a54c6f35f6204fd226df8e95df45b05435d3 → d98ee4ecfcdc337ba74ec9a008dd58a17239cfde 

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

ce30faf  reordered documentation, as requested by chapoton

21abe06  replaced '... ' by '....:'

4121914  improve the subword complex, in particular its documentation

b2b7f68  more intelligent fix a la stump

d662af6  Revert "improve the subword complex, in particular its documentation"

1ef2de7  Revert "first version of the subword complexes as of 2015"

03b60e2  added two doctests

2a34bef  added more doctests

d98ee4e  added more doctests, many more to come

comment:74 Changed 8 years ago by
Commit:  d98ee4ecfcdc337ba74ec9a008dd58a17239cfde → 294d5f8f513b74bae5de22221b069a21df1ed6f4 

Branch pushed to git repo; I updated commit sha1. New commits:
294d5f8  fixed a docstring

comment:75 Changed 8 years ago by
Commit:  294d5f8f513b74bae5de22221b069a21df1ed6f4 → eddaef5ada286366d9dc8d1f5af329f915c9da76 

comment:76 Changed 8 years ago by
Commit:  eddaef5ada286366d9dc8d1f5af329f915c9da76 → fb852f57be8fe8897ee2c2bad7b4847c865476e8 

Branch pushed to git repo; I updated commit sha1. New commits:
fb852f5  added back the references to __init__

comment:77 Changed 8 years ago by
Commit:  fb852f57be8fe8897ee2c2bad7b4847c865476e8 → 3783ea23dc6a102eea745f59cd2a6b48fb87d5ca 

Branch pushed to git repo; I updated commit sha1. New commits:
3783ea2  added some stuff that got lost during rebasing

comment:78 Changed 8 years ago by
Commit:  3783ea23dc6a102eea745f59cd2a6b48fb87d5ca → 43f1dcedca38ef310502c66f4bc9147040d2972a 

Branch pushed to git repo; I updated commit sha1. New commits:
c07e76e  moved from ComplexReflectionGroup/CoxeterGroupChevie to ReflectionGroup

f845089  change in irreducible components

bbe138f  Merge branch 't/18620/9a3e5cdacac40b505f7aad0a62b9e1bec76d8f3a' into u/stumpc5/11187new

43f1dce  reflection groups in the reference

comment:79 Changed 8 years ago by
Commit:  43f1dcedca38ef310502c66f4bc9147040d2972a → 2f737ce0ee193dc7de8b89586dc3939f2686c161 

Branch pushed to git repo; I updated commit sha1. New commits:
2f737ce  got the documentation run again as expected

comment:80 Changed 8 years ago by
Commit:  2f737ce0ee193dc7de8b89586dc3939f2686c161 → 9df16f6d63f7107669e9229211f9520aeb39a7ad 

Branch pushed to git repo; I updated commit sha1. New commits:
9df16f6  cleaned the documentation of the complex reflection group category

comment:81 Changed 8 years ago by
Commit:  9df16f6d63f7107669e9229211f9520aeb39a7ad → bfd9aa684127059d97777ad55432772322a78ebc 

Branch pushed to git repo; I updated commit sha1. New commits:
bfd9aa6  added some references to ComplexReflectionGroups

comment:82 Changed 8 years ago by
Commit:  bfd9aa684127059d97777ad55432772322a78ebc → e3b047bff2bd12f99a3fbea87f5417cd2c012745 

Branch pushed to git repo; I updated commit sha1. New commits:
e3b047b  added some references to ComplexReflectionGroups

comment:83 Changed 8 years ago by
Description:  modified (diff) 

Keywords:  days 64.5 added 
comment:84 Changed 8 years ago by
Commit:  e3b047bff2bd12f99a3fbea87f5417cd2c012745 → d6736681a50cfae4bcd58ad601af43860b5733e3 

comment:85 Changed 8 years ago by
Commit:  d6736681a50cfae4bcd58ad601af43860b5733e3 → ef3cbe7303af54c2390e04404fb6a4c2623b9c3e 

Branch pushed to git repo; I updated commit sha1. New commits:
ef3cbe7  fixed a docstring

comment:86 followup: 87 Changed 8 years ago by
Hello Christian, thanks for your hard work on this ticket. I have only had a quick look, so trivial remarks:
 not yet 100% coverage, I think (I have seen a few methods without examples)
TODO::
should be.. TODO::
 Could you please replace
Returns
byReturn
everywhere.  Sometime the first line of the doc is made of two sentences. Please separate them as:
Return something. Something is very very important.
 Please try to keep the lines 80 characters long.
 Bad alignement of EXAMPLES:: in
distinguished_reflection
,distinguished_reflections
 missing optional in
sage: W = CoxeterGroup(["A",4], implementation="chevie")
And you should aim to have a green light from the patchbot. So far, this has never been the case.
comment:87 Changed 8 years ago by
Replying to chapoton:
Thanks for having a look at this ticket!
I have only had a quick look, so trivial remarks
I am aware of the stuff to do in the documentation, but before getting there (or, at least, in parallel, I'll fix your complaints now), I try to get the overall setting right (this includes the category framework, the interaction with other reflection group classes, and the dependence of chevie).
And you should aim to have a green light from the patchbot. So far, this has never been the case.
I'd like to get it green, but it has not run in 5 days, and I can only fix stuff the patchbot complaints about if I see its results regularly after uploading a new commit sequence (a patchbot that doesn't run soon after uploading doesn't really help during a coding sprint)...
Thanks again, Christian
comment:88 Changed 8 years ago by
There are many patchbots, but only one that has added all your participants to its trust list (at my demand). This one is currently not active. You can try to contact some active patchbots owners to ask them to trust the participants (see http://wiki.sagemath.org/buildbot/owners and http://patchbot.sagemath.org/machines). Or run your own patchbot.
comment:89 Changed 8 years ago by
There are many patchbots, but only one that has added all your participants to its trust list (at my demand).
Oh, I didn't know that all participants must be on the trust list, I thought it was only the submitter of the branch  I gonna try to get a patchbot running...
comment:90 Changed 8 years ago by
Yes, this comes from some difficulty to identify the true authors of the code..
For patchbot, please see the instructions there : http://wiki.sagemath.org/buildbot/details
comment:91 Changed 8 years ago by
Commit:  ef3cbe7303af54c2390e04404fb6a4c2623b9c3e → 40d22b23447993f976e9680aec23d32c4f4c34f4 

comment:92 Changed 8 years ago by
I just started a patchbot recently, and I believe I now have it configured to trust all relevant participants and prioritize this ticket.
First run through wasn't very helpful, since I don't have Chevie installed. Though presumably everything should be marked with some kind of optional tag so that tests are skipped if Chevie isn't installed.
I'm in the process of trying to install Chevie, and if I succeed, I'll have it do another run.
comment:93 Changed 8 years ago by
I have Gap3/Chevie installed now.
It's probably easier (and a lot faster) to run
./sage t ./src/sage/combinat/root_system/reflection_group_complex.py
,
and if things are crashing and you need to see where, then do
./sage t verbose ./src/sage/combinat/root_system/reflection_group_complex.py
.
Patchbots are good for finding merge conflicts with new betas and seeing if you unintentionally broke other parts of Sage, but at this point I think it's sufficient to only test the one file you're changing with this ticket.
After playing around for a bit, something seems to be broken with the iterator. The example W = ReflectionGroup((1,1,3))
(should be isomorphic to S3) included as a test works fine, but if I try W = ReflectionGroup((3,1,1))
(should be isomorphic to cyclic group of order 3) then it keeps printing out the three elements in a neverending loop.
comment:95 Changed 8 years ago by
When adding new files, you should follow http://doc.sagemath.org/html/en/developer/coding_basics.html#headingsofsagelibrarycodefiles, in particular use the correct license statement.
comment:96 followup: 97 Changed 8 years ago by
Thanks for having a look here! I am currently working (since yesterday) on speeding the iteration through the group, I hope I have gotten somewhere later today...
I guess I must tag each and every test as
# optional  chevie
which means that these tests are not run on patchbots per default, or do they run if the patchbot has chevie installed?
comment:97 followup: 98 Changed 8 years ago by
Replying to stumpc5:
I guess I must tag each and every test
Not literally every test, there are probably tests which do not require chevie
.
do they run if the patchbot has chevie installed?
They would be run if the Sage optional package chevie
is installed. However, such a package does not exist.
comment:98 followup: 99 Changed 8 years ago by
Replying to jdemeyer:
Not literally every test, there are probably tests which do not require
chevie
.
Every test (except for the creation of the category of complex reflection groups) handle with reflection groups (no surprise), and even initializing such a group requires chevie
.
But within finite time, I will work on making some core functionalities work without having chevie
installed.
They would be run if the Sage optional package
chevie
is installed. However, such a package does not exist.
Is that a problem? Should there be a possibility of installing chevie
as an optional package? I update the description of this ticket to explain how to get chevie installed in a system. See #8906 for old stuff about that.
comment:99 Changed 8 years ago by
Replying to stumpc5:
Is that a problem? Should there be a possibility of installing
chevie
as an optional package?
It's not a problem for me or for this ticket. I guess it is a problem if people want to use the code that you are writing here, since getting chevie
to work within Sage is nontrivial.
comment:100 Changed 8 years ago by
Description:  modified (diff) 

comment:101 Changed 8 years ago by
Commit:  40d22b23447993f976e9680aec23d32c4f4c34f4 → 1c64c9b599944baabbecea7267854493546687c7 

Branch pushed to git repo; I updated commit sha1. New commits:
5a205a1  moved right coset representatives

25139dd  removed the preloading of the ucf for reflection groups, and moved the new iterator into real reflection groups.

7df41c2  new iterators through group

83f9893  updated licence statements

1c64c9b  added a todo for the iterator

comment:102 Changed 8 years ago by
Commit:  1c64c9b599944baabbecea7267854493546687c7 → 43f505667c936dc07ed42a653383e844f2555cab 

comment:103 Changed 8 years ago by
Okay, got 100% coverage and all tests passing! But I wouldn't know how to get the startup module green as I turned the ComplexReflectionGroups
category into a super category of CoxeterGroups
which is loaded on startup.
comment:104 followup: 105 Changed 8 years ago by
I have a failing doctest for is_regular
. There's not a problem with the is_regular
method, but for W = ReflectionGroup(23)
, there are two elements where my computer returns a different reduced word than expected ( https://www.diffchecker.com/ewvevrda ). I consistently get the same different answer. Are you using the older/more stable gapjm4, or the latest gapjm5 ?
Unrelated, but I also can't get the long_element() method to work.
comment:105 Changed 8 years ago by
Replying to kdilks:
for
W = ReflectionGroup(23)
, there are two elements where my computer returns a different reduced word than expected
This computation is currently done purely in my code, and it's not related to chevie
. The iterator comes from _iterator_tracking_words
so let's see what happens there.
Related here, and indeed also to your question on long_element
, observe that currently
sage: W = ReflectionGroup(23); W Irreducible complex reflection group of rank 3 and type H3 sage: W.category() Join of Category of finite permutation groups and Category of irreducible finite coxeter groups sage: W = ReflectionGroup(['H',3]); W Irreducible real reflection group of rank 3 and type H3 sage: W.category() Join of Category of finite permutation groups and Category of irreducible finite coxeter groups
Since the iter algorithm for coxeter groups is slightly smarter (it takes into account that the weak order is graded), the two iter functions are different.
But:
 both iter functions are supposed to return the same iteration through the group
 both iter functions should always return the elements in the very same order (in contrary to what we see on my vs on your machine)
I found a bug (having a set
rather than a list
in one spot), so let's see if this is fixed with my next push in a few minutes after all tests pass here.
Unrelated, but I also can't get the long_element() method to work.
The reason is, that you constructed the real group as a complex group, so the long element for ReflectionGroup(['H',3])
works while it currently doesn't work for ReflectionGroup(23)
. I do have to fix that and be more careful constructing the right class instance.
comment:106 Changed 8 years ago by
Commit:  43f505667c936dc07ed42a653383e844f2555cab → 29a3fa3b0f4f303db3f0ec6c0835a902563a4812 

comment:107 Changed 8 years ago by
Commit:  29a3fa3b0f4f303db3f0ec6c0835a902563a4812 → b123a8170cbb025cfdeb6cb783b0b49854848f32 

comment:110 Changed 8 years ago by
Commit:  b123a8170cbb025cfdeb6cb783b0b49854848f32 → ab724b4cb5560b44007591ef873eb30c840dd08f 

Branch pushed to git repo; I updated commit sha1. New commits:
5f8f940  improved the reduced word for reflection groups

711cf2c  8906: create file ``type`` declaring spkg optional

f2777b8  8906: create packageversion.txt file containing the version

c5addcf  8906: generate checksums.ini

af16d79  8906: create SPKG.txt file with package description, etc.

ddbe1d5  8906: create spkginstall script, patches and increment patch level

0defcc1  8906: add gap3 commandline option to sage startup script

ab724b4  merged latest develop

comment:111 Changed 8 years ago by
Commit:  ab724b4cb5560b44007591ef873eb30c840dd08f → 963397d991abbb26605eb76cf7b3a895ba90d0ac 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
1c64c9b  added a todo for the iterator

d0bfd81  fixed many doctests coming from new iteration order

43f5056  reached 100% coverage and all tests passing

e056f65  iteration is now stable among machines

29a3fa3  reordered some doctests according to the new iteration order

4a13365  trivialities

556ab0e  changed the reduced word computation for real groups

b123a81  hope the iteration order is now stable, also added a few optional chevie, but not many

5f8f940  improved the reduced word for reflection groups

963397d  merged develop

comment:112 Changed 7 years ago by
Commit:  963397d991abbb26605eb76cf7b3a895ba90d0ac → 7fd11b569246a9bc351b31e84c8c65e529b9f4aa 

Branch pushed to git repo; I updated commit sha1. New commits:
7fd11b5  Merge branch 'develop' into u/stumpc5/11187new

comment:113 Changed 7 years ago by
Dependencies:  #18620 → #18620, #8906 

comment:114 Changed 7 years ago by
Commit:  7fd11b569246a9bc351b31e84c8c65e529b9f4aa → 64431778120d52faa686e33c23ac0ac6c9d472a3 

Branch pushed to git repo; I updated commit sha1. New commits:
6443177  Merge branch 'develop' into u/stumpc5/11187new

comment:115 Changed 7 years ago by
Commit:  64431778120d52faa686e33c23ac0ac6c9d472a3 → 3f66a41fc017ceaa8806eaba8db27207e511ac2b 

comment:116 Changed 7 years ago by
Commit:  3f66a41fc017ceaa8806eaba8db27207e511ac2b → dd250eb43799a4d1f7670614611a9d002956955f 

comment:117 Changed 7 years ago by
Commit:  dd250eb43799a4d1f7670614611a9d002956955f → a00d9c7c47a8689219b9ae8ae723c2b4afa890d5 

Branch pushed to git repo; I updated commit sha1. New commits:
a00d9c7  sage_eval to eval in one instance, needs to be checked at some point

comment:118 Changed 7 years ago by
Commit:  a00d9c7c47a8689219b9ae8ae723c2b4afa890d5 → 8c4204a0d70b13061de5d6602f43692bc627ec24 

Branch pushed to git repo; I updated commit sha1. New commits:
8c4204a  bug fix for the fundamental invariants

comment:119 Changed 7 years ago by
Commit:  8c4204a0d70b13061de5d6602f43692bc627ec24 → 7bb0e6cca3dd78380cfaa176f5e5384b626cb4b0 

comment:120 Changed 7 years ago by
Milestone:  sage6.8 → sage7.1 

comment:121 Changed 7 years ago by
Commit:  7bb0e6cca3dd78380cfaa176f5e5384b626cb4b0 → 3957a563983601f143659b1a843fc656ca4a0ae3 

Branch pushed to git repo; I updated commit sha1. New commits:
20d254c  fixed the same bug in two more places

fa40e5c  Merge branch 'u/stumpc5/19912' into u/stumpc5/11187new

1df9f53  working on a few glitches for CRGs

66ec881  merged 7.1.beta0

3957a56  Merge branch 'u/stumpc5/19912' into u/stumpc5/11187new

comment:122 Changed 7 years ago by
Dependencies:  #18620, #8906 → #18620, #8906, #19912 

comment:123 Changed 7 years ago by
Commit:  3957a563983601f143659b1a843fc656ca4a0ae3 → 530a5c802620230b461820cb737f59dedb335f16 

Branch pushed to git repo; I updated commit sha1. New commits:
530a5c8  added two implementations of the invariant forms

comment:124 Changed 7 years ago by
Commit:  530a5c802620230b461820cb737f59dedb335f16 → 8b39c853ac1d85e90e4ed84633c777a2e63ff701 

comment:125 Changed 7 years ago by
Commit:  8b39c853ac1d85e90e4ed84633c777a2e63ff701 → aa87d02482826ef4211c06e9fe2ebc13048fc482 

Branch pushed to git repo; I updated commit sha1. New commits:
aa87d02  added matrix representation for nonwellgenerated types

comment:126 Changed 7 years ago by
Commit:  aa87d02482826ef4211c06e9fe2ebc13048fc482 → 186d38745e7dbde9f619d7152a48bc33065c77ab 

Branch pushed to git repo; I updated commit sha1. New commits:
186d387  Merge branch 'u/stumpc5/11187new' into test

comment:127 Changed 7 years ago by
Commit:  186d38745e7dbde9f619d7152a48bc33065c77ab → 094c608b5eda8d16f1b84af1f439614e4f869a2e 

Branch pushed to git repo; I updated commit sha1. New commits:
094c608  added some more todos for character theory

comment:128 Changed 7 years ago by
Dependencies:  #18620, #8906, #19912 → #20107 

comment:129 followup: 130 Changed 7 years ago by
are you aware of Geck's PyCox? http://www.mathematik.unistuttgart.de/~geckmf/chv1r6180.py
comment:130 Changed 7 years ago by
Cc:  Jean Michel added 

Replying to dimpase:
are you aware of Geck's PyCox? http://www.mathematik.unistuttgart.de/~geckmf/chv1r6180.py
Sure, but it is limited to finite _real_ reflection groups, while I certainly want to work with real and with complex reflection groups. E.g., the results in
 http://jlms.oxfordjournals.org/content/90/3/919.abstract (paywall)
 http://arxiv.org/abs/1211.2789 (free)
were only possible by using the code in this ticket to work in Sage with complex reflection groups from chevie through the gap3 interface.
comment:131 Changed 7 years ago by
Commit:  094c608b5eda8d16f1b84af1f439614e4f869a2e → 3da1ebb8a47bdd4ff2c36761b15e663c6d2485fe 

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

181a352  undoing one thing in spkgsrc

ec01e30  moved to experimental package

6e599cf  renamed optional package to experimental package in interfaces/gap3.py

1583f99  replaced #20107 by :trac:20107

38dbd60  replaced optional chevie by optional gap3 in combinat/root_system/coxeter_group.py

d4f3a1d  updated the spkgsrc to delete gap3 packages that need compilation

7696a2c  added patch for init.g and fixed typo in patch for gap.sh

3d123df  merged 20107 to the newest develop

3da1ebb  Merge branch 'u/stumpc5/11187' into tmp

comment:132 Changed 7 years ago by
Concerning the iteration through elements without keeping them in memory, we have to benchmark agains
gap> i:=0;; gap> W:=CoxeterGroup("E",7);; gap> ForEachElement(W,function(x)i:=i+1; > if i mod 1000000=0 then Print("*\c");fi; > end);Print("\n"); **
which takes about a second per million elements, see https://webusers.imjprg.fr/~jean.michel/gap3/htm/chap075.htm.
comment:133 Changed 7 years ago by
Branch:  u/stumpc5/11187new → u/vripoll/11187new 

comment:134 followup: 135 Changed 7 years ago by
Cc:  Florent Hivert added 

Commit:  3da1ebb8a47bdd4ff2c36761b15e663c6d2485fe → e2f919fa6529b2444dbec4cfffde671b8927d1b7 
Milestone:  sage7.1 → sage7.2 
Continuing the discussion from #13580.
Replying to hivert:
Replying to tscrim:
Christian, Vivien, and I are working on trying to speed up the iteration of finite Coxeter groups by using (subgroups of) permutation groups and the iterator method of generic Coxeter groups, which uses a
SearchForest
(see #11187 and theCoxeterGroups
category). Our goal is to try and match or beat GAP3 iteration time (which we are about ~30x slower).I'll be both very happy and interested to have usecase for this code.
This will have the more serious use case I know of (the code in the generic Coxeter groups iterator, e.g., via CoxeterGroup
, has not had any serious optimization attempts AFAIK).
We are in the process pushing our successor computation code to Cython and some of our other methods, but we would appreciate any insights you have.
Do you have a typical example relying on
SearchForest
of code on top of #11187 that I can profile ? Note that I included in the documentation of map_reduce.py a guide on how to profile the parallel code. I'd like to measure the proportion of time that is spend on Coxeter code one one hand and on the parallel infrastructure on the other.
Our current test cases are B_{6} (~0.5s) and E_{7} (~1m).
Now, concerning coxeter group, I'm quite sure that rewriting part of the code in C/C++ and using advanced parallel stuff (AVX instruction set + SIMD) you can get speedup a large as several hundreds and even thousand ! As a witness on https://github.com/hivert/IVMPG I wrote a code whose goal is to enumerate integer vector modulo permutation group. Compared to the code we have in Sage which has been Cythonized and optimized by Simon King I manage to get speedup a large as x2000 ! I need help on the build system to be able to distribute it. I'm pretty sure the code there is very similar to what you need in Coxeter groups.
One of the thoughts I'm bouncing around is doing a full C/C++ version using arrays, and then using Cython (mainly PermutationGroupElement
) as a transfer layer to Sage/GAP. In which case it probably should be split off as a little specialized spkg.
That is quite an impressive speedup. I know a little bit about build systems (and its on my todo to learn more for fixing up coxeter3/LiE too), but, e.g., Jeroen or François are good people to ask. Although if you need GCC 5.x, then we will need to do some changes to Sage. Yet, I think moving to GCC 5.x is something that we want and are going to do soon...
New commits:
e2f919f  New iterator for real reflection groups.

comment:135 Changed 7 years ago by
Replying to tscrim:
One of the thoughts I'm bouncing around is doing a full C/C++ version using arrays, and then using Cython (mainly
PermutationGroupElement
) as a transfer layer to Sage/GAP. In which case it probably should be split off as a little specialized spkg.
This is very similar to what I did. The idea is that if you want large performance, you need to completely control the way you use the memory. So what I did is to build in parallel a large linked list of arrays and then thanks to Cython I returns Sage an iterator which is a view on this list which wrap the element of the linked list into proper Sage Element.
comment:136 Changed 7 years ago by
Branch:  u/vripoll/11187new → u/stumpc5/11187new 

comment:137 Changed 7 years ago by
Commit:  e2f919fa6529b2444dbec4cfffde671b8927d1b7 → 48835b7db7daaf22eb903f146140d06b44dda872 

Branch pushed to git repo; I updated commit sha1. New commits:
48835b7  Merge branch 'develop' into u/stumpc5/11187

comment:138 Changed 7 years ago by
Branch:  u/stumpc5/11187new → u/vripoll/11187new 

comment:139 Changed 7 years ago by
Commit:  48835b7db7daaf22eb903f146140d06b44dda872 → 1b798a72cdd02cfa90c59ec0f9dd555b8b06e33a 

For the record: as discussed last night, the first goal would be to separate the category stuff into its own ticket (@tscrim is working on this), and then to get the present version well documented, tested, and finished. Only after this work is done, we will start improving stuff and add functionality.
To this end, we have to:
 get the category stuff right
 test properly that reducible groups work as desired
 test properly that the labelling system of simples/hyperplanes/reflections works properly
 test that the assumptions on how we obtain the data from chevie is correct (one example here is that the code tries to get n simple reflections, N* distinguished reflections, and then the remaining (NN*) other reflections when listing all reflections. This has to be rechecked, but there are plenty of more similar examples.
New commits:
e75f1a2  Updated TODO list

1b798a7  Merge branch 'u/stumpc5/11187new' of git://trac.sagemath.org/sage into t/11187/11187new

comment:140 Changed 7 years ago by
Branch:  u/vripoll/11187new → u/chapoton/11187 

Commit:  1b798a72cdd02cfa90c59ec0f9dd555b8b06e33a → 0fc36ce6f8ae66d792fd8197313786d961f37fa4 
comment:141 Changed 7 years ago by
Commit:  0fc36ce6f8ae66d792fd8197313786d961f37fa4 → 220e0f03ed025daeb6372bee28477882afee6861 

Branch pushed to git repo; I updated commit sha1. New commits:
220e0f0  trac #11187 changing next to python3 syntax

comment:142 Changed 7 years ago by
Commit:  220e0f03ed025daeb6372bee28477882afee6861 → 3270a98685af6ab39f2c3e13f695a5dd226227d6 

Branch pushed to git repo; I updated commit sha1. New commits:
3270a98  trac #11187 remove a method reflection to root in complex groups

comment:143 Changed 7 years ago by
Commit:  3270a98685af6ab39f2c3e13f695a5dd226227d6 → b947ea518e2152eb903d88f459c183a85d3f3587 

Branch pushed to git repo; I updated commit sha1. New commits:
b947ea5  trac #11187 adding doc for reflecting hyperplane

comment:144 Changed 7 years ago by
Commit:  b947ea518e2152eb903d88f459c183a85d3f3587 → a697817ab730832f10a06ee6a1ff878be7060dd3 

Branch pushed to git repo; I updated commit sha1. New commits:
a697817  trac #11187 remove the invariant linear form in complex, for the moment

comment:145 Changed 7 years ago by
Dependencies:  #20107 → #20107 #20397 

comment:146 Changed 7 years ago by
Commit:  a697817ab730832f10a06ee6a1ff878be7060dd3 → 9b73808d144d03319e2bde278e07f55f3e34741e 

Branch pushed to git repo; I updated commit sha1. New commits:
9b73808  trac #11187 correct a doctest in coxeter_groups

comment:147 Changed 7 years ago by
Commit:  9b73808d144d03319e2bde278e07f55f3e34741e → 114b20b8cc80c613f656262d495441edc02a9261 

Branch pushed to git repo; I updated commit sha1. New commits:
114b20b  trac #11187 adding a doctest for the invariant form

comment:148 Changed 7 years ago by
Commit:  114b20b8cc80c613f656262d495441edc02a9261 → 4e80c52a448ead8b5072d1ba0a342de1ec116098 

Branch pushed to git repo; I updated commit sha1. New commits:
71b77a1  Merge branch 'develop' into u/tscrim/11187

b171922  Merge branch 'u/stumpc5/11187new' of git://trac.sagemath.org/sage into u/tscrim/11187

67e882a  Trying to get a bit more speed out and doing some code cleanup.

c76dd9d  Merge branch 'u/tscrim/11187' of git://trac.sagemath.org/sage into u/stumpc5/11187

ea14eb8  fixed some doctests, fixed a bug in hyperplanes, work on the iterator

4e80c52  Merge branch 'u/chapoton/11187' of git://trac.sagemath.org/sage into u/stumpc5/11187

comment:149 Changed 7 years ago by
Commit:  4e80c52a448ead8b5072d1ba0a342de1ec116098 → d7343219c7e12dd870605967b9be27222a27a2df 

Branch pushed to git repo; I updated commit sha1. New commits:
d734321  trac #11187 a typo in doc in complex reflection groups

comment:150 Changed 7 years ago by
Commit:  d7343219c7e12dd870605967b9be27222a27a2df → 7d88a6a49bd3f6332e7a3f71b59739d3aeb5d843 

Branch pushed to git repo; I updated commit sha1. New commits:
7d88a6a  trac #11187 again a minor doc typo in complex refl group

comment:151 Changed 7 years ago by
Commit:  7d88a6a49bd3f6332e7a3f71b59739d3aeb5d843 → f42aba99d6597cfb7c849900cba284b43816f64e 

Branch pushed to git repo; I updated commit sha1. New commits:
f42aba9  trac #11187 a typo in doc of realreflgroups

comment:152 Changed 7 years ago by
Branch:  u/chapoton/11187 → u/tscrim/11187 

Commit:  f42aba99d6597cfb7c849900cba284b43816f64e → e3b52f7780447e5cfb208bfc12bd088f36d3cf47 
New commits:
19af4e7  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

b7f5ffe  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

a384a06  Pulling changes from #11187 and making tests use colored permutations.

e3b52f7  Merge branch 'public/categories/complex_reflection_groups20397' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

comment:153 Changed 7 years ago by
Branch:  u/tscrim/11187 → u/vripoll/11187new 

Commit:  e3b52f7780447e5cfb208bfc12bd088f36d3cf47 → 1b798a72cdd02cfa90c59ec0f9dd555b8b06e33a 
comment:154 Changed 7 years ago by
Commit:  1b798a72cdd02cfa90c59ec0f9dd555b8b06e33a → 2b01e8df1562bb6415635f784f8423cea3af8fd8 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
c76dd9d  Merge branch 'u/tscrim/11187' of git://trac.sagemath.org/sage into u/stumpc5/11187

ea14eb8  fixed some doctests, fixed a bug in hyperplanes, work on the iterator

4e80c52  Merge branch 'u/chapoton/11187' of git://trac.sagemath.org/sage into u/stumpc5/11187

d734321  trac #11187 a typo in doc in complex reflection groups

7d88a6a  trac #11187 again a minor doc typo in complex refl group

f42aba9  trac #11187 a typo in doc of realreflgroups

b7f5ffe  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

a384a06  Pulling changes from #11187 and making tests use colored permutations.

e3b52f7  Merge branch 'public/categories/complex_reflection_groups20397' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

2b01e8d  Merge branch 'u/tscrim/11187' of git://trac.sagemath.org/sage into t/11187/11187new

comment:155 Changed 7 years ago by
Branch:  u/vripoll/11187new → u/stumpc5/11187new 

comment:156 Changed 7 years ago by
Branch:  u/stumpc5/11187new → u/tscrim/11187 

Commit:  2b01e8df1562bb6415635f784f8423cea3af8fd8 → 6daedef45c6888f596b803977897aeec9512e4ac 
Dependencies:  #20107 #20397 → #20107 
Everything should work now up to the printing of wellgenerated as part of the finite Coxeter group category.
Last 10 new commits:
f77e89f  Getting the degrees correct.

14d4c39  Merge branch 'public/20395' into public/categories/complex_reflection_groups20397

79bbb3f  Merge branch 'public/categories/complex_reflection_groups20397' into u/tscrim/reflection_groups11187

a7c3db8  Fixing up the categories and doing some other cleanup.

b9e0b0d  working on the iterator

823ab4f  Merge branch 'u/chapoton/11187' of git://trac.sagemath.org/sage into u/stumpc5/11187

80fe997  finished to encorporate the new iterator

3538599  Merge branch 'u/vripoll/11187new' of git://trac.sagemath.org/sage into u/stumpc5/11187

90244d3  Merge branch 'u/stumpc5/11187new' of git://trac.sagemath.org/sage into u/tscrim/reflection_groups11187

6daedef  Fixing everything else up except for wellgenerated being added to finite Coxeter groups.

comment:157 Changed 7 years ago by
Branch:  u/tscrim/11187 → u/chapoton/11187 

Commit:  6daedef45c6888f596b803977897aeec9512e4ac → 5f568f95708bd417947b28de3bf2806d423c947b 
New commits:
5f568f9  trac #11187 fixing shepard groups

comment:158 Changed 7 years ago by
Branch:  u/chapoton/11187 → u/stumpc5/11187 

comment:159 Changed 7 years ago by
Branch:  u/stumpc5/11187 → u/tscrim/11187 

Commit:  5f568f95708bd417947b28de3bf2806d423c947b → d4775c7541edc49cc4aaace14edeec15255db700 
I was able to stop GeneralizedCoxeterGroup
, ComplexReflectionGroup
, and FiniteCoxeterGroup
from being imported during startup.
New commits:
a7976cb  removed ComplexReflectionGroups from global namespace

d4775c7  Removing FiniteCoxeterGroups, ComplexReflectionGroups, and GeneralizedWeylGroups from startup import.

comment:160 Changed 7 years ago by
Commit:  d4775c7541edc49cc4aaace14edeec15255db700 → beadf8a46bccffc7934b1fd15d607561be937642 

Branch pushed to git repo; I updated commit sha1. New commits:
beadf8a  Lazy imports must have absolute paths.

comment:161 Changed 7 years ago by
Commit:  beadf8a46bccffc7934b1fd15d607561be937642 → 185be8f64c6f3d265615b3773c1ef27378a72d2a 

Branch pushed to git repo; I updated commit sha1. New commits:
f7a6dd1  Fixing global namespace issues.

a53d8f1  added somes lines in doc + correction typo shephard

4e71ef7  Some typos and corrections in the descriptions

535d37e  removed spaces in example

185be8f  Merge branch 'public/categories/complex_reflection_groups20397' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

comment:162 Changed 7 years ago by
Commit:  185be8f64c6f3d265615b3773c1ef27378a72d2a → b81dc4216eeba5b4d4efa4929f8d2a10c05f7da2 

comment:163 Changed 7 years ago by
Branch:  u/tscrim/11187 → u/chapoton/11187 

Commit:  b81dc4216eeba5b4d4efa4929f8d2a10c05f7da2 → 2d70fabf62f9e613ffc9bf488e0dd3e9698ce02c 
some tiny fixes in doc, mainly
New commits:
2b0c33a  trac #11187 replace exec by sage_eval, add more doc on reflection groups

7271ead  trac #11187 fixing the doc (again)

298cf98  trac #1118è adding one doc

dffa31e  trac #11187 some more fine doc tuning

2ec03b0  trac #11187 working on doc

7324754  trac #11187 still working on doc

2d70fab  trac #11187 more doc tuning

comment:164 Changed 7 years ago by
in the cython iterator, we should use sth like
tuple( W.simple_reflection(W._index_set_inverse[i]) for i in xrange(len(W._index_set)) )
comment:165 Changed 7 years ago by
Branch:  u/chapoton/11187 → u/tscrim/11187 

Commit:  2d70fabf62f9e613ffc9bf488e0dd3e9698ce02c → 5b1f2b0353e134ee7413a9b3dee3ffa3347ed7ae 
New commits:
0bcce62  Making the iterator even faster.

a04b9c8  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

9c96d33  added ST type casting for real groups + documentation of crg's

5b1f2b0  Merge branch 'u/stumpc5/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

comment:166 Changed 7 years ago by
Commit:  5b1f2b0353e134ee7413a9b3dee3ffa3347ed7ae → d4256691756e16c64b6f2f855367f5afb911a00c 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
aa07666  trac #11187 fixing catalog

b2ba34c  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

fa1b496  Changing the indexing sets and how they are handled.

a1b61e6  moderate pep8 changes

6da6f94  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

85d0ce1  fixed a typo in the doc

ab7afba  marked a missing doctest + fixed a bug to show type C2

72082b1  fixed a bug in noncrossing partition lattice and reflection/absolute order

844f450  Merge branch 'u/stumpc5/11187' of trac.sagemath.org:sage into u/tscrim/reflection_groups11187

d425669  Fixing failures and some other things.

comment:167 Changed 7 years ago by
Branch:  u/tscrim/11187 → u/nthiery/11187 

comment:168 Changed 7 years ago by
Branch:  u/nthiery/11187 → u/stumpc5/11187 

Commit:  d4256691756e16c64b6f2f855367f5afb911a00c → e074050131cd483022c5783f749cbce3ac522ae5 
New commits:
87cd989  fixed one doctest in category complex reflection group

05a8ab4  11187: fixed trivial doctest failures

32bf1ae  11187: cleanup of the organization of the various axioms (WellGenerated, ...) for complex/generalized reflection groups + documentation improvements

73276ca  Merge branch 'u/tscrim/11187' of trac.sagemath.org:sage into t/11187/11187

e074050  merged

comment:169 Changed 7 years ago by
Commit:  e074050131cd483022c5783f749cbce3ac522ae5 → b572c74c3a4db2909bea5c538c8be26089068fc6 

Branch pushed to git repo; I updated commit sha1. New commits:
b572c74  fixed a missing doctest

comment:170 Changed 7 years ago by
Only two trivial doctests failing + the missing doctest for _test_well_generated
, though Nicolas thought he had added it.
comment:171 Changed 7 years ago by
Branch:  u/stumpc5/11187 → u/nthiery/11187 

comment:172 Changed 7 years ago by
Commit:  b572c74c3a4db2909bea5c538c8be26089068fc6 → 4e4e65ed2613a4486b34219f66a34ba40183cf58 

Branch pushed to git repo; I updated commit sha1. New commits:
24191b0  11187: factored out common code from GeneralizedCoxeterGroups and ComplexReflectionGroups

fced757  trac #11187 fixing import of QQbar and prod

1daeaea  trac #11187 fixing degrees for finite Coxeter and Weyl

2791d1e  Merge branch 'u/chapoton/11187' of trac.sagemath.org:sage into t/11187/11187

4e4e65e  11187: finished the generic irreducible_components; fixed trivial doctest failures

comment:173 Changed 7 years ago by
Commit:  4e4e65ed2613a4486b34219f66a34ba40183cf58 → 6d88d923ee9bbf2a86c223f4aec60ddad4e2eb6f 

Branch pushed to git repo; I updated commit sha1. New commits:
6d88d92  11187: oops, forgot to add one file

comment:174 Changed 7 years ago by
Branch:  u/nthiery/11187 → public/11187 

comment:175 Changed 7 years ago by
Commit:  6d88d923ee9bbf2a86c223f4aec60ddad4e2eb6f → 9f72be34c793f93cb0cc48f03ad7b2849ffabcad 

Branch pushed to git repo; I updated commit sha1. New commits:
9f72be3  Moving the iterator for complex reflection groups to the Cython file.

comment:176 Changed 7 years ago by
Commit:  9f72be34c793f93cb0cc48f03ad7b2849ffabcad → ef835f68696526ba2a9dff934b65e45a1f095219 

comment:177 Changed 7 years ago by
Commit:  ef835f68696526ba2a9dff934b65e45a1f095219 → a45d823a6544c48bdb903b143338e3d5fbbccd36 

Branch pushed to git repo; I updated commit sha1. New commits:
a45d823  fixed the allowed input types for reflection groups

comment:178 Changed 7 years ago by
Commit:  a45d823a6544c48bdb903b143338e3d5fbbccd36 → a5e263dba2601bb006e2b7510d35c0f656603a8c 

Branch pushed to git repo; I updated commit sha1. New commits:
a9d858a  11187: documentation, crosslinks, full subcategories, uniformization of simple reflections/reflections/distinguished reflections

211baa4  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

a5e263d  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

comment:179 Changed 7 years ago by
Commit:  a5e263dba2601bb006e2b7510d35c0f656603a8c → a95a5a91f8c164cc863df9103665d90c056e7624 

Branch pushed to git repo; I updated commit sha1. New commits:
a95a5a9  11187: updated doctest + added comment for the next time

comment:180 Changed 7 years ago by
Commit:  a95a5a91f8c164cc863df9103665d90c056e7624 → e654c3ed7751689b839547dec558fc9e375716a7 

Branch pushed to git repo; I updated commit sha1. New commits:
e654c3e  11187: updated doctests: Coxeter groups don't define additional structure since they have now a supercategory doing so

comment:181 Changed 7 years ago by
Commit:  e654c3ed7751689b839547dec558fc9e375716a7 → c3b5a320e409080e967f44b4d4aa30715b06abfe 

Branch pushed to git repo; I updated commit sha1. New commits:
c3b5a32  fixed a bug in the degrees in coxeter groups category

comment:182 Changed 7 years ago by
Commit:  c3b5a320e409080e967f44b4d4aa30715b06abfe → b5b8c3b2103a53cc49060ddb584fcd8048e5deab 

Branch pushed to git repo; I updated commit sha1. New commits:
b5b8c3b  trac #11187 adding codegrees to finite Coxeter groups

comment:183 Changed 7 years ago by
Commit:  b5b8c3b2103a53cc49060ddb584fcd8048e5deab → 7eab06d2a6f8af4e837342b695c7d479fe140e55 

Branch pushed to git repo; I updated commit sha1. New commits:
7eab06d  trac #11187 some typos in doc of categories/complex_reflection_group

comment:184 Changed 7 years ago by
Commit:  7eab06d2a6f8af4e837342b695c7d479fe140e55 → fae677f17b4af97addae261a4b5882ddd99a19d1 

Branch pushed to git repo; I updated commit sha1. New commits:
98f3c76  11187: fixed TODO statement, removed unused import

d43a39f  11187: removed commented out link for some_elements

e34c9a7  11187: refactor from_reduced_word and apply_*_reflection, cleanup

8e08db2  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

fae677f  11187: removed 3 spurious empty lines in the doc

comment:185 Changed 7 years ago by
Commit:  fae677f17b4af97addae261a4b5882ddd99a19d1 → f048ac3d8b012474f16174ded8a23b5143db9ea0 

comment:186 Changed 7 years ago by
Commit:  f048ac3d8b012474f16174ded8a23b5143db9ea0 → 7880f309bc060c42fcbdfa8e276011ce0ce88c22 

Branch pushed to git repo; I updated commit sha1. New commits:
7880f30  trac #11187 one failing doctest in categories

comment:187 Changed 7 years ago by
Commit:  7880f309bc060c42fcbdfa8e276011ce0ce88c22 → 9b7aba28093f4f811acecc67264cb011f029f299 

Branch pushed to git repo; I updated commit sha1. New commits:
568d6c4  11187: Generalized methods from ComplexReflectionGroup, replace apply_simple_reflection_* by a definition of simple_reflection, documentation, TODOs

9b7aba2  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

comment:188 Changed 7 years ago by
Commit:  9b7aba28093f4f811acecc67264cb011f029f299 → fc5f6671bf504b8a6ee073b9e7d9aba888220b03 

Branch pushed to git repo; I updated commit sha1. New commits:
fc5f667  removed several cached methods

comment:189 Changed 7 years ago by
Commit:  fc5f6671bf504b8a6ee073b9e7d9aba888220b03 → b55ecb03098c873ac669c5f185c319b0d42fbbca 

Branch pushed to git repo; I updated commit sha1. New commits:
b55ecb0  moved the reduced_word_in_ref for real groups there, removed a few more cached methods, documented a few optional arguments

comment:190 Changed 7 years ago by
Commit:  b55ecb03098c873ac669c5f185c319b0d42fbbca → 2aa89b2f08edb3a5351b610f56ef9a40196f9076 

Branch pushed to git repo; I updated commit sha1. New commits:
2aa89b2  commenting on the todo list

comment:191 Changed 7 years ago by
Commit:  2aa89b2f08edb3a5351b610f56ef9a40196f9076 → a7d65c6893b0026f2a680fc38fb51168136b0824 

Branch pushed to git repo; I updated commit sha1. New commits:
a7d65c6  fixed one failing doctest

comment:192 Changed 7 years ago by
Commit:  a7d65c6893b0026f2a680fc38fb51168136b0824 → eb45b27d0c5e184572a7719ea22b86c65211349a 

comment:193 Changed 7 years ago by
Commit:  eb45b27d0c5e184572a7719ea22b86c65211349a → f4801f2edbfcf87d1b1302a406945afaae6dbb37 

Branch pushed to git repo; I updated commit sha1. New commits:
f4801f2  trac 11187 making degrees be integers

comment:194 Changed 7 years ago by
Commit:  f4801f2edbfcf87d1b1302a406945afaae6dbb37 → cb0a7a0509d90826d120ae17f108d3ab15ef9b02 

Branch pushed to git repo; I updated commit sha1. New commits:
cb0a7a0  trac 11187 degrees in tuple

comment:195 Changed 7 years ago by
Commit:  cb0a7a0509d90826d120ae17f108d3ab15ef9b02 → 55104aac8bac1952f924ae0198a7c79a7682ee0f 

comment:196 Changed 7 years ago by
Commit:  55104aac8bac1952f924ae0198a7c79a7682ee0f → fbdabdc58bb28e12f36a8251df683d2b496df619 

comment:197 Changed 7 years ago by
Commit:  fbdabdc58bb28e12f36a8251df683d2b496df619 → e22ee7ecb32cd5914f6dbb69593d240d4de1e9eb 

Branch pushed to git repo; I updated commit sha1. New commits:
8542562  11187: add _test_degrees (not yet finished)

256edb7  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

b29b702  11187: reflecting_hyperplanes > reflection_hyperplanes

ebbf9bf  11187: further work on _test_(co)_degrees; reflecting_hyperplanes > reflection_hyperplanes

249f619  Merge branch 'public/11187' of trac.sagemath.org:sage into t/11187/11187

e22ee7e  11187: fixed typo in the code of number_of_irreducible_components

comment:198 Changed 7 years ago by
Commit:  e22ee7ecb32cd5914f6dbb69593d240d4de1e9eb → 722a7675cec4cb22aab3ec594fca457c8ec2da57 

comment:199 Changed 7 years ago by
Commit:  722a7675cec4cb22aab3ec594fca457c8ec2da57 → 5294cf3cf5dcfd92d95f606697a45abc9844405b 

Branch pushed to git repo; I updated commit sha1. New commits:
5294cf3  11187: fixed codegrees for colored permutations

comment:200 Changed 7 years ago by
Commit:  5294cf3cf5dcfd92d95f606697a45abc9844405b → c7425f8e27bff3d13e05bd9d9a880e14817ccee9 

Branch pushed to git repo; I updated commit sha1. New commits:
c7425f8  trac 11187 a typo in doc

comment:201 Changed 7 years ago by
Commit:  c7425f8e27bff3d13e05bd9d9a880e14817ccee9 → 2def1e83ba323899fa1a95391a1b17a0bda4140c 

comment:202 Changed 7 years ago by
Commit:  2def1e83ba323899fa1a95391a1b17a0bda4140c → 241e6607cf62f3938019d5118aac018e9254dc24 

Branch pushed to git repo; I updated commit sha1. New commits:
241e660  11187: sum > ZZ.sum for consistency when the list of degrees is empty

comment:203 Changed 7 years ago by
Commit:  241e6607cf62f3938019d5118aac018e9254dc24 → 82ff2882bcdee03518224a3b63a0f2ebd1dd33b3 

Branch pushed to git repo; I updated commit sha1. New commits:
82ff288  11187: rename 'reflecting hyperplane' to 'reflection hyperplane' in the docstrings

comment:204 Changed 7 years ago by
Commit:  82ff2882bcdee03518224a3b63a0f2ebd1dd33b3 → c09cd4d5e8f61909a57b74dfa09627f6aef6f130 

Branch pushed to git repo; I updated commit sha1. New commits:
c09cd4d  11187: Fixed heisenbug caused by temporarily breaking the implementation of (co)degrees for testing without restoring it after

comment:205 Changed 7 years ago by
Commit:  c09cd4d5e8f61909a57b74dfa09627f6aef6f130 → 0becba3698653fedcfa8547d44fc7d45fb2fecb3 

Branch pushed to git repo; I updated commit sha1. New commits:
cf00014  trac #20396 adding the invariant linear form for complex refl groups

75e8d69  trac #20396 first try at simpler implementation

cdef4c3  trac #20396 not yet working

8013693  Merge branch 'public/20396' of git://trac.sagemath.org/sage into u/stumpc5/20396

e02cf02  still in trouble about the complex case

07bd053  merged 11187

bf2ea92  fixed the coroots + work on invariant form

0becba3  Merge branch 'public/11187' of git://trac.sagemath.org/sage into u/stumpc5/20396

comment:206 Changed 7 years ago by
Commit:  0becba3698653fedcfa8547d44fc7d45fb2fecb3 → 8927ab98d9aa88610774567e4bde0ce8ec8c14b9 

Branch pushed to git repo; I updated commit sha1. New commits:
8927ab9  matrix action is on the right!

comment:208 Changed 7 years ago by
@tscrim: For the record, the timings for iteration in E7 seem to be odd:
sage: W = ReflectionGroup(['E',7]) sage: %time for w in W.iteration(algorithm="depth",tracking_words=False): pass CPU times: user 3.71 s, sys: 0 ns, total: 3.71 s Wall time: 3.79 s sage: %time for w in W.iteration(algorithm="breadth",tracking_words=False): pass CPU times: user 8.72 s, sys: 4.84 ms, total: 8.73 s Wall time: 8.72 s sage: %time for w in W.iteration(algorithm="depth",tracking_words=True): pass CPU times: user 5.83 s, sys: 0 ns, total: 5.83 s Wall time: 5.81 s sage: %time for w in W.iteration(algorithm="breadth",tracking_words=True): pass CPU times: user 20.6 s, sys: 67.4 ms, total: 20.6 s Wall time: 20.7 s
Also, I discussed with Nicolas yesterday, and (as what I said on the weekend), we are back at the situation that one can be much smarter concerning the ascents: Let i be an ascent of w and si and s_j commute. Then i is also an ascent of wsj. I will try to use that today, but it still seems that any test in this direction is much slower than doing the actual test for an ascent the way we already do it.
comment:209 Changed 7 years ago by
Commit:  8927ab98d9aa88610774567e4bde0ce8ec8c14b9 → def701588736edda81a86ddd1a9cbdbc849657be 

comment:210 followup: 212 Changed 7 years ago by
Travis, could you have a look at the diffset 8497f5d; I commented out that the change is actually used, see #for i in self.noncom[first]
, it seems to slow down the test even though much less is actually tested. Maybe you see how to get rid of the overhead the line for i in self.noncom[first]
has compared to for i in range(first+1,self.n)
.
comment:211 followup: 213 Changed 7 years ago by
Travis: Also, you introduced _reduced_word
to be a lazy attribute. This has a few drawbacks: not self._reduced_word is None
in the length function in reflection_group_real.py
is never triggered, and _reduced_word
is now either a word in the index set (if the lazy attribute is triggered in the real case), or a word in range(n)
if one has gotten it through the iterator. This way, the method reduced_word
in the real case is always executed, even though the reduced word is already known.
Two solutions are:
 either set
_reduced_word
to a word in the indices in the iterator for the cost of list lookups for each letter in the word  write the lazy attribute in the real case so that it gives a word in
range(n)
 do neither and forget about the info gotten from the iterator in the real case.
comment:212 Changed 7 years ago by
Replying to stumpc5:
Travis, could you have a look at the diffset 8497f5d; I commented out that the change is actually used, see
#for i in self.noncom[first]
, it seems to slow down the test even though much less is actually tested. Maybe you see how to get rid of the overhead the linefor i in self.noncom[first]
has compared tofor i in range(first+1,self.n)
.
My guess is because Cython is smart enough to translate
for i in range(n)
into
for(i=0; i<n; ++i)
whereas for the other, it is iterating over some object (which, a priori, may not even be iterable). In more detail, Cython doesn't even know that it is iterating over a list, just that self.noncom
is. Try:
nc = self.noncom[first] for i in nc:
or the variant
nc = self.noncom[first] k = len(nc) for dummy in range(k): i = nc[dummy]
where you have cdef list nc
and cdef int dummy, k
(although I suspect the former of these is faster).
comment:213 Changed 7 years ago by
Replying to stumpc5:
Travis: Also, you introduced
_reduced_word
to be a lazy attribute. This has a few drawbacks:not self._reduced_word is None
in the length function inreflection_group_real.py
is never triggered, and_reduced_word
is now either a word in the index set (if the lazy attribute is triggered in the real case), or a word inrange(n)
if one has gotten it through the iterator. This way, the methodreduced_word
in the real case is always executed, even though the reduced word is already known.
Doing this simplified the code and probably got us a small speedup. Yet, this is a bug in my implementation. However, I'm not sure how much it is worth it to compute the length of an arbitrary element without computing its reduced word. If it becomes that time critical for someone, we can get them an algorithm
option for length()
.
This way, the method
reduced_word
in the real case is always executed, even though the reduced word is already known.
This doesn't make sense. If anything reduced_word()
will throw an error if _reduced_word
was done by the lazy_attribute
. The iterator overwrites (the lazy attribute) _reduced_word
if the word comes from there, so it does not get computed twice. Otherwise it was a user created word and will never be identical (not just equals) to an element created from the iterator.
Two solutions are:
 write the lazy attribute in the real case so that it gives a word in
range(n)
This is what I would do. Although we might be better off just doing our own implementation using a list (which we mutate) and starting with self.domain()
.
comment:214 Changed 7 years ago by
Commit:  def701588736edda81a86ddd1a9cbdbc849657be → 5a377c42bdcba53fde3504489eab396194bc509f 

comment:215 Changed 7 years ago by
Done with "write the lazy attribute in the real case so that it gives a word in range(n)". This is now moved to cython (speed gain ~100 times faster).
comment:216 followup: 218 Changed 7 years ago by
One big issue that is still waiting for me to fix it is that it remains unclear if we want a left or right action. The implementation is a left action, but e.g. #20402 assumes a right action. I will get to this before the weekend and would then hope for the code to become stable and some final reviewing :)
comment:217 Changed 7 years ago by
Just FYI  you have reverted a python3 compatible change with this:
 ....: print('%s %s'%(w.reduced_word(), w.is_regular(W.coxeter_number()))) + ....: print w.reduced_word(), w.is_regular(h)
I think 2to3
can handle this, but I'm just stating a paranoia about it.
Although I would advocate for making doctests that avoid printing through an iteration altogether (really annoying failures when the iteration order changes), but this isn't really a big issue.
I will take a more detailed look at the recent changes before the weekend.
comment:218 Changed 7 years ago by
Replying to stumpc5:
One big issue that is still waiting for me to fix it is that it remains unclear if we want a left or right action. The implementation is a left action, but e.g. #20402 assumes a right action. I will get to this before the weekend and would then hope for the code to become stable and some final reviewing :)
How much additional code would it take to make it work on both sides, allowing the user to do ReflectionGroup(..., action="right")
?
comment:219 followups: 220 222 Changed 7 years ago by
Just FYI  you have reverted a python3 compatible change with this
I am sure there are reasons for that python3 convention, but I do prefer simple to read code. Anyway, I can also do the other if you prefer.
How much additional code would it take to make it work on both sides,
I was afraid you'd ask that. in my eyes: too much. The elements are considered as permutations, words, and matrices at the same time (and there are even two types of words, in simples and in all reflections, and the simple words have wired properties in the nonreal case), making options would pollute the code just soo much, I think, and would introduce so many possible mistakes. I prefer to have clean and fast code, that is hopefully getting better than chevie, eventually. Once that is achieved, I am getting in favour with adding another layer of user friendliness.
comment:220 Changed 7 years ago by
Replying to stumpc5:
I was afraid you'd ask that.
:)
in my eyes: too much. The elements are considered as permutations, words, and matrices at the same time (and there are even two types of words, in simples and in all reflections, and the simple words have wired properties in the nonreal case), making options would pollute the code just soo much, I think, and would introduce so many possible mistakes. I prefer to have clean and fast code, that is hopefully getting better than chevie, eventually. Once that is achieved, I am getting in favour with adding another layer of user friendliness.
Fair enough: let's focus on getting things to work.
One thing though is that a lot of features are completely symmetric (e.g. the reverse of a word is a word). The only spot where things differ is when there is an explicit action. In my experience when implementing representations of semigroups, it was often possible to abstracted everything out by manipulating elements in the code as functions whenever there is an action involved (sorry, I know, this is a big vague).
Of course, a prerequisite for implementing this seamlessly is that permutation groups themselves would allow for both left and right actions.
Cheers,
Nicolas
comment:221 Changed 7 years ago by
Commit:  5a377c42bdcba53fde3504489eab396194bc509f → 8862849a09e7f9ecadbb181e5051edf1e9924b73 

Branch pushed to git repo; I updated commit sha1. New commits:
8862849  simple_roots, simple_coroots, and fundamental_weights are now index by the index set

comment:222 followup: 224 Changed 7 years ago by
Replying to stumpc5:
Just FYI  you have reverted a python3 compatible change with this
I am sure there are reasons for that python3 convention, but I do prefer simple to read code. Anyway, I can also do the other if you prefer.
In python3, print
becomes an actual function, so it needs the parentheses (which due to Python's handling of parentheses, print(foo)
gets evaluated to print foo
, but having a comma means (foo, bar)
becomes a tuple). So to help make the doctests 2/3 compatible and to ease the eventual transition, I do prefer the way I had it or make it into a few examples and have a doctest which does check everything (without verbose).
comment:223 Changed 7 years ago by
Commit:  8862849a09e7f9ecadbb181e5051edf1e9924b73 → ef83323a827a477c3c7330fedb429e4dd737e044 

Branch pushed to git repo; I updated commit sha1. New commits:
ef83323  clarified the fundamental weights, including an example

comment:224 Changed 7 years ago by
Replying to tscrim:
In python3,
I see, doesn't make it easier to read though...
comment:225 Changed 7 years ago by
Commit:  ef83323a827a477c3c7330fedb429e4dd737e044 → 5ecc970883b3ed30314b35949956a7eba135b8ff 

Branch pushed to git repo; I updated commit sha1. New commits:
5ecc970  got the action on roots and the inversion set in its left and rightflavor

comment:226 Changed 7 years ago by
Commit:  5ecc970883b3ed30314b35949956a7eba135b8ff → 42d6b753c5c5cb4e1e82cf0ed3520768c5dc5d6b 

Branch pushed to git repo; I updated commit sha1. New commits:
42d6b75  made matrix an alias for to_matrix, as matrix returns a huge permutation matrix of the roots

comment:227 Changed 7 years ago by
Commit:  42d6b753c5c5cb4e1e82cf0ed3520768c5dc5d6b → 8810c41ad280c06b307f059220c03f046eafa0d7 

Branch pushed to git repo; I updated commit sha1. New commits:
8810c41  fixed one doctest + work on the invariant form

comment:228 Changed 7 years ago by
Commit:  8810c41ad280c06b307f059220c03f046eafa0d7 → 2ba374053a128ead4f7288b8b302a2752211a870 

Branch pushed to git repo; I updated commit sha1. New commits:
2ba3740  fixed another doctest + a typo

comment:229 Changed 7 years ago by
Commit:  2ba374053a128ead4f7288b8b302a2752211a870 → e28bbecbd3a0b08b35f8892874dcd4d7d7f61e4e 

Branch pushed to git repo; I updated commit sha1. New commits:
e28bbec  edited the length function to always use the reduced word

comment:230 Changed 7 years ago by
Commit:  e28bbecbd3a0b08b35f8892874dcd4d7d7f61e4e → dc0106628d58cb263866b9cd601696ea71f18e9f 

comment:231 Changed 7 years ago by
Commit:  dc0106628d58cb263866b9cd601696ea71f18e9f → fd0c9cc616a03de20a12aba530c96e73f8f60911 

Branch pushed to git repo; I updated commit sha1. New commits:
fd0c9cc  added optional gap3

comment:232 Changed 7 years ago by
Commit:  fd0c9cc616a03de20a12aba530c96e73f8f60911 → 1537a795f6d7a86f8c3a7a79e86d4b31405de323 

Branch pushed to git repo; I updated commit sha1. New commits:
1537a79  added optional gap3

comment:233 Changed 7 years ago by
Commit:  1537a795f6d7a86f8c3a7a79e86d4b31405de323 → a60cf31d47777353bcae92665d5e5e739fc02a29 

Branch pushed to git repo; I updated commit sha1. New commits:
a60cf31  added some missing optional gap3

comment:234 Changed 7 years ago by
Commit:  a60cf31d47777353bcae92665d5e5e739fc02a29 → 4955d68d9e4dbde2d1425afbae36e619c2d6ba14 

comment:235 Changed 7 years ago by
Commit:  4955d68d9e4dbde2d1425afbae36e619c2d6ba14 → c5c43bcba449039892cecb0d18238b4bf87e8a7f 

Branch pushed to git repo; I updated commit sha1. New commits:
c5c43bc  the next round of optional gap3 insertions

comment:236 Changed 7 years ago by
Dependencies:  #20107 

Description:  modified (diff) 
Keywords:  days64.5 days80 added; days 64.5 removed 
Status:  needs_work → needs_review 
comment:237 Changed 7 years ago by
Description:  modified (diff) 

comment:238 Changed 7 years ago by
Commit:  c5c43bcba449039892cecb0d18238b4bf87e8a7f → f541349eae7b8b3b6a61c57aaafd6bb8c84b3265 

comment:239 Changed 7 years ago by
I've done a few little beautification things. If my changes are good, then we can set this to a positive review.
comment:240 Changed 7 years ago by
Thanks Travis, looking forward to have this to positive review and then merged!
comment:242 Changed 7 years ago by
FYI  Patchbots seem happy with it.
yes, but it didn't test the gap3 stuff again (I just asked Frederic how to get the patchbot to also test those), and also we still have two new startup modules (though the patchbot sees them without indicating it, see http://patchbot.sagemath.org/log/11187/Ubuntu/14.04/x86_64/3.13.085generic/librae/20160420%2010:00:02?plugin=plugins.startup_modules&diff=/log/0/Ubuntu/14.04/x86_64/3.13.085generic/librae/20160412%2018%3A16%3A59&ticket=11187&base=7.2.beta4)
comment:243 followup: 245 Changed 7 years ago by
To now run the doctests # optional  gap3
, it seems I cannot use the patchbot anymore. Should I then do instead
sagepatchbot/sage tp 4 all long optional=gap3
myself?
comment:244 Changed 7 years ago by
I think that will be best. I was going to try this afternoon to see if I can avoid importing those categories at startup, but I might not be successful at it.
comment:245 followup: 246 Changed 7 years ago by
Replying to stumpc5:
To now run the doctests
# optional  gap3
, it seems I cannot use the patchbot anymore. Should I then do insteadsagepatchbot/sage tp 4 all long optional=gap3myself?
I'd use optional=gap3,sage
comment:246 Changed 7 years ago by
Replying to dimpase:
I'd use
optional=gap3,sage
wow, you are silently reading these hundreds of comments to now help out  impressive, thanks!
comment:247 Changed 7 years ago by
To remove generalized_coxeter_groups.py
from the startup would either require making it a lazy import in coxeter_groups.py
or making coxeter_groups.py
not being imported at startup. I think the better strategy is to leave things how they are as an incentive to have coxeter_groups.py
not be imported at startup. (Also it is only a net +1 imported at startup.)
comment:248 Changed 7 years ago by
Commit:  f541349eae7b8b3b6a61c57aaafd6bb8c84b3265 → 0db28d6c6e64818eb32fb9826ff7a41333c814d6 

Branch pushed to git repo; I updated commit sha1. New commits:
0db28d6  fixed doctests that have not been run last week!

comment:249 Changed 7 years ago by
I also copied the other optional from the patchbot:
./sage tp 6 all long optional=ccache,gap3,mpir,patchbot,python2,sage
@Frederic, is that what I should do? Would it be hard (desirable?) to tell the patchbot (maybe in config_file.json
) which optional tests should in general be run?
comment:250 Changed 7 years ago by
there are still a few doctest failures that need to be fixed:
stumpc5@findstat:~/webfindstat/sagepatchbot$ ./sage tp 6 long optional=ccache,gap3,mpir,patchbot,python2,sage src/sage/combinat/root_system/coxeter_group.py too many failed tests, not using stored timings Running doctests with ID 20160421131355f420497c. Git branch: t/11187/public/11187 Using optional=ccache,gap3,mpir,patchbot,python2,sage Doctesting 1 file using 6 threads. sage t long src/sage/combinat/root_system/coxeter_group.py ********************************************************************** File "src/sage/combinat/root_system/coxeter_group.py", line 205, in sage.combinat.root_system.coxeter_group.CoxeterGroupAsPermutationGroup.__init__ Failed example: TestSuite(W).run() # optional  gap3 Expected nothing Got: Failure in _test_well_generated: Traceback (most recent call last): File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/misc/sage_unittest.py", line 283, in run test_method(tester = tester) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/categories/finite_complex_reflection_groups.py", line 628, in _test_well_generated tester.assertEqual(self.number_of_simple_reflections(), self.rank()) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python/unittest/case.py", line 515, in assertEqual assertion_func(first, second, msg=msg) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python/unittest/case.py", line 508, in _baseAssertEqual raise self.failureException(msg) AssertionError: 3 != 27  The following tests failed: _test_well_generated ********************************************************************** File "src/sage/combinat/root_system/coxeter_group.py", line 231, in sage.combinat.root_system.coxeter_group.CoxeterGroupAsPermutationGroup._element_class Failed example: W._element_class() is W.element_class # optional  gap3 Exception raised: Traceback (most recent call last): File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 858, in compile_and_execute exec(compiled, globs) File "<doctest sage.combinat.root_system.coxeter_group.CoxeterGroupAsPermutationGroup._element_class[1]>", line 1, in <module> W._element_class() is W.element_class # optional  gap3 File "sage/structure/parent.pyx", line 854, in sage.structure.parent.Parent.__getattr__ (/srv/data/findstat.imp.fuberlin.de/sagepatchbot/src/build/cythonized/sage/structure/parent.c:8083) attr = getattr_from_other_class(self, self._category.parent_class, name) File "sage/structure/misc.pyx", line 253, in sage.structure.misc.getattr_from_other_class (/srv/data/findstat.imp.fuberlin.de/sagepatchbot/src/build/cythonized/sage/structure/misc.c:1763) raise dummy_attribute_error AttributeError: 'CoxeterMatrixGroup_with_category' object has no attribute '_element_class' ********************************************************************** 2 items had failures: 1 of 4 in sage.combinat.root_system.coxeter_group.CoxeterGroupAsPermutationGroup.__init__ 1 of 3 in sage.combinat.root_system.coxeter_group.CoxeterGroupAsPermutationGroup._element_class [54 tests, 2 failures, 10.25 s]  sage t long src/sage/combinat/root_system/coxeter_group.py # 2 doctests failed
comment:251 Changed 7 years ago by
And then, there are also a few in gap3.py
, I am not sure if we have to treat them, but if anyone knows how to do it, please go ahead (this is also necessary to get the package to be optional rather than experimental...
stumpc5@findstat:~/webfindstat/sagepatchbot$ ./sage tp 6 long optional=ccache,gap3,mpir,patchbot,python2,sage src/sage/interfaces/gap3.py too many failed tests, not using stored timings Running doctests with ID 2016042113153552e24ead. Git branch: t/11187/public/11187 Using optional=ccache,gap3,mpir,patchbot,python2,sage Doctesting 1 file using 6 threads. sage t long src/sage/interfaces/gap3.py ********************************************************************** File "src/sage/interfaces/gap3.py", line 393, in sage.interfaces.gap3.Gap3._execute_line Failed example: f([1,2,3]) #optional  gap3 Expected: Traceback (most recent call last): ... RuntimeError: Gap3 produced error output Error, List Element: <position> must be a positive integer at return L[0] ... in ... called from main loop brk> quit; ... Got: <BLANKLINE> Traceback (most recent call last): File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 858, in compile_and_execute exec(compiled, globs) File "<doctest sage.interfaces.gap3.Gap3._execute_line[3]>", line 1, in <module> f([Integer(1),Integer(2),Integer(3)]) #optional  gap3 File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 805, in __call__ return getattr(P, self.name())(*args) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 607, in __call__ return self._parent.function_call(self._name, list(args), kwds) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/gap.py", line 921, in function_call res = self.eval(marker+cmd) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/gap.py", line 573, in eval result = Expect.eval(self, input_line, **kwds) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/expect.py", line 1239, in eval for L in code.split('\n') if L != '']) File "/srv/data/findstat.imp.fuberlin.de/sagepatchbot/local/lib/python2.7/sitepackages/sage/interfaces/gap.py", line 771, in _eval_line raise RuntimeError(message) RuntimeError: Gap3 produced error output Error, Function: <function> must be a function <BLANKLINE> executing __SAGE_LAST__:="__SAGE_LAST__";;x(sage0);; ********************************************************************** File "src/sage/interfaces/gap3.py", line 440, in sage.interfaces.gap3.Gap3.help Failed example: m #optional  gap3 Expected: [ [ 1, 2, 3 ], [ 4, 5, 6 ] ] Got: <BLANKLINE> ********************************************************************** File "src/sage/interfaces/gap3.py", line 442, in sage.interfaces.gap3.Gap3.help Failed example: m.Print() #optional  gap3 Expected: [ [ 1, 2, 3 ], [ 4, 5, 6 ] ] Got: "__SAGE_LAST__" ********************************************************************** File "src/sage/interfaces/gap3.py", line 446, in sage.interfaces.gap3.Gap3.help Failed example: m #optional  gap3 Expected: [ [ 1, 2, 3 ], [ 4, 5, 6 ] ] Got: <BLANKLINE> ********************************************************************** File "src/sage/interfaces/gap3.py", line 448, in sage.interfaces.gap3.Gap3.help Failed example: m.Print() #optional  gap3 Expected: [ [ 1, 2, 3 ], [ 4, 5, 6 ] ] Got: "__SAGE_LAST__" ********************************************************************** 2 items had failures: 1 of 5 in sage.interfaces.gap3.Gap3._execute_line 4 of 10 in sage.interfaces.gap3.Gap3.help [108 tests, 5 failures, 7.01 s]  sage t long src/sage/interfaces/gap3.py # 5 doctests failed  Total time for all tests: 15.7 seconds cpu time: 2.2 seconds cumulative wall time: 7.0 seconds
}}}
comment:252 Changed 7 years ago by
Commit:  0db28d6c6e64818eb32fb9826ff7a41333c814d6 → 7afc02c78f5c0aa58d5b638e20c135678a3bec7e 

Branch pushed to git repo; I updated commit sha1. New commits:
7afc02c  Fixing doctests in combinat/root_system/coxeter_group.py.

comment:253 followup: 256 Changed 7 years ago by
not a good idea in general to use a bare except:
except:
rather find what exception it is supposed to catch
comment:254 Changed 7 years ago by
I fixed the errors in combinat/root_system/coxeter_group.py
(to which, we should be able to do most (all) of that functionality without appealing to GAP3 and we should probably fold that into (Irreducible)RealReflectionGroup
at some point). The latter errors are indicating issues with the interface, so I would just wait on those until we do #20393. Since gap3
is an experimental package, we are not strictly required to have all # optional  gap3
doctests pass.
comment:255 Changed 7 years ago by
Commit:  7afc02c78f5c0aa58d5b638e20c135678a3bec7e → 02694256c73520e7f4dd80c455cdae133be858a7 

Branch pushed to git repo; I updated commit sha1. New commits:
0269425  Blanket fix bare except block.

comment:256 followup: 258 Changed 7 years ago by
Replying to chapoton:
not a good idea in general to use a bare except:
except:rather find what exception it is supposed to catch
Right, good catch. I'm going to try and find the precise error that gets raised, but I've pushed a hack fix for now.
comment:257 Changed 7 years ago by
Commit:  02694256c73520e7f4dd80c455cdae133be858a7 → 9b0ef927c17789f2dbeea432b8316f99d138cb88 

Branch pushed to git repo; I updated commit sha1. New commits:
9b0ef92  Making it an AttributeError.

comment:258 Changed 7 years ago by
Replying to tscrim:
Replying to chapoton:
not a good idea in general to use a bare except:
except:rather find what exception it is supposed to catch
Right, good catch. I'm going to try and find the precise error that gets raised, but I've pushed a hack fix for now.
It only throws an AttributeError
, and I have changed it to catch only that.
comment:259 Changed 7 years ago by
Thanks for the fixes, I am doing another round of doctests now...
comment:260 followup: 261 Changed 7 years ago by
okay, the only remaining failures are
sage t long src/sage/tests/cmdline.py # 1 doctest failed sage t long src/sage/repl/interpreter.py # 1 doctest failed sage t long src/sage/parallel/map_reduce.py # 1 doctest failed sage t long src/sage/interfaces/gap3.py # 5 doctests failed sage t long src/sage/repl/interface_magic.py # 1 doctest failed
of which we are only responsible for gap3.py
. I'd thus suggest to set this to positive review...
comment:261 followup: 262 Changed 7 years ago by
Replying to stumpc5:
okay, the only remaining failures are
sage t long src/sage/tests/cmdline.py # 1 doctest failed sage t long src/sage/repl/interpreter.py # 1 doctest failed sage t long src/sage/parallel/map_reduce.py # 1 doctest failed sage t long src/sage/interfaces/gap3.py # 5 doctests failed sage t long src/sage/repl/interface_magic.py # 1 doctest failedof which we are only responsible for
gap3.py
. I'd thus suggest to set this to positive review...
Hi travis,
Can you send me the log of the map_reduce.py
error ? On what kinds of machine was it ? Is it reproducible ?
Florent
comment:262 Changed 7 years ago by
Replying to hivert:
Replying to stumpc5:
okay, the only remaining failures are
sage t long src/sage/tests/cmdline.py # 1 doctest failed sage t long src/sage/repl/interpreter.py # 1 doctest failed sage t long src/sage/parallel/map_reduce.py # 1 doctest failed sage t long src/sage/interfaces/gap3.py # 5 doctests failed sage t long src/sage/repl/interface_magic.py # 1 doctest failedof which we are only responsible for
gap3.py
. I'd thus suggest to set this to positive review...Hi travis,
...christian.
Can you send me the log of the
map_reduce.py
error ? On what kinds of machine was it ? Is it reproducible ?
I didn't log the test, but I can restart it now. If it stays, I will give you the details.
comment:263 followup: 264 Changed 7 years ago by
all tests pass this time, sorry for not being able to get you the error...
comment:264 followup: 265 Changed 7 years ago by
Replying to stumpc5:
all tests pass this time, sorry for not being able to get you the error...
Please let me know if it happen again.
comment:265 Changed 7 years ago by
comment:266 Changed 7 years ago by
Commit:  9b0ef927c17789f2dbeea432b8316f99d138cb88 → 8d765204ab597ed2fd1570c2438d5fda3eb60192 

Branch pushed to git repo; I updated commit sha1. New commits:
8d76520  using a cached version self._number_of_reflections to avoid computing it again and again, expecially important in has_descent

comment:267 Changed 7 years ago by
Should I actually at all still push stuff into this ticket, or should we rather get it to positive review and do everything else in individual tickets? (I have redone the doctest for reflection_group_real.py
after this last change.)
comment:268 Changed 7 years ago by
Please stop touching anything here if you want this to be positive reviewed soon and not in 4 more years.
comment:269 Changed 7 years ago by
if you want this to be positive reviewed soon and not in 4 more years.
That's a little harsh for my taste. Anyway, I agree that I should stop doing anything here, the list of improvements is at #20394.
comment:271 Changed 7 years ago by
I would rebase this to beta5
, run the patchbot and do all doctests with the gap3 flag one more time.
comment:272 Changed 7 years ago by
Commit:  8d765204ab597ed2fd1570c2438d5fda3eb60192 → 0b6d895c865ebdb319de6d58e862e7ae1a1cb784 

Branch pushed to git repo; I updated commit sha1. New commits:
0b6d895  Merge branch 'develop' into public/11187

comment:273 Changed 7 years ago by
Authors:  Christian Stump → Christian Stump, Frédéric Chapoton, Nicolas Thiery, Travis Scrimshaw 

Reviewers:  → Christian Stump, Frédéric Chapoton, Nicolas Thiery, Vivian Ripoll, Travis Scrimshaw 
comment:274 Changed 7 years ago by
Authors:  Christian Stump, Frédéric Chapoton, Nicolas Thiery, Travis Scrimshaw → Christian Stump, Frédéric Chapoton, Nicolas Thiéry, Travis Scrimshaw 

Reviewers:  Christian Stump, Frédéric Chapoton, Nicolas Thiery, Vivian Ripoll, Travis Scrimshaw → Christian Stump, Frédéric Chapoton, Nicolas Thiéry, Vivien Ripoll, Travis Scrimshaw 
comment:275 Changed 7 years ago by
Authors:  Christian Stump, Frédéric Chapoton, Nicolas Thiéry, Travis Scrimshaw → Christian Stump, Frédéric Chapoton, Nicolas M. Thiéry, Travis Scrimshaw 

Reviewers:  Christian Stump, Frédéric Chapoton, Nicolas Thiéry, Vivien Ripoll, Travis Scrimshaw → Christian Stump, Frédéric Chapoton, Nicolas M. Thiéry, Vivien Ripoll, Travis Scrimshaw 
comment:276 Changed 7 years ago by
Status:  needs_review → positive_review 

ok, guys, I think it is time. I am setting to positive review
in the name of all authors/reviewers.
comment:278 Changed 7 years ago by
Branch:  public/11187 → 0b6d895c865ebdb319de6d58e862e7ae1a1cb784 

Resolution:  → fixed 
Status:  positive_review → closed 
Apply only trac_11187finite_reflection_groupscs.patch