Opened 8 years ago

Last modified 3 years ago

#11187 closed enhancement

Implementation of finite reflection groups — at Version 100

Reported by: stumpc5 Owned by: tbd
Priority: major Milestone: sage-7.2
Component: combinatorics Keywords: reflection group, days49, days64.5, days80
Cc: vripoll, kdilks, SimonKing, jmichel, hivert Merged in:
Authors: Christian Stump Reviewers:
Report Upstream: N/A Work issues:
Branch: u/stumpc5/11187-new (Commits) Commit: 40d22b23447993f976e9680aec23d32c4f4c34f4
Dependencies: #18620 Stopgaps:

Description (last modified by stumpc5)

This patch implements reflection groups to work with finite (crystallographic, real, complex) reflection groups. It is based on the GAP3 package chevie.

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]

To install chevie, one needs to download it from Jean Michel's website http://webusers.imj-prg.fr/~jean.michel/gap3/, update some paths in GAP3_folder/bin/gap.sh and make this file execute when typing gap3 inside the sage shell. I wouldn't think this is superhard to make an optional package as discussed at #8906.

Change History (101)

comment:1 Changed 8 years ago by stumpc5

  • Component changed from PLEASE CHANGE to combinatorics
  • Status changed from new to needs_work

comment:2 Changed 8 years ago by stumpc5

Apply only trac_11187-finite_reflection_groups-cs.patch

comment:3 Changed 8 years ago by stumpc5

  • Status changed from needs_work to needs_review

Apply only trac_11187-finite_reflection_groups-cs.patch

comment:4 Changed 8 years ago by stumpc5

  • Dependencies set to 8327
  • Description modified (diff)

comment:5 Changed 8 years ago by stumpc5

  • Status changed from needs_review to needs_work

comment:6 Changed 8 years ago by stumpc5

Apply only trac_11187-finite_reflection_groups-cs.patch

comment:7 Changed 8 years ago by stumpc5

  • Dependencies changed from 8327 to #8327

comment:8 Changed 8 years ago by stumpc5

  • Milestone set to sage-4.7.2

Changed 6 years ago by stumpc5

comment:9 Changed 6 years ago by stumpc5

+    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 6 years ago by tscrim

  • Dependencies changed from #8327 to #8327 #14137

I've factored out the CartanMatrix class into #14137.

comment:11 follow-up: Changed 6 years ago by tscrim

  • Keywords days49 added

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.

comment:12 in reply to: ↑ 11 Changed 6 years ago by stumpc5

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 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:16 follow-up: Changed 5 years ago by chapoton

Hello Christian

Do you have a git branch for this ticket somewhere ? or should I try to resurrect the patch ?

comment:17 in reply to: ↑ 16 Changed 5 years ago by stumpc5

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 5 years ago by chapoton

Hello, no, no real need for this code, just asking a general question. Now that #10963 is over.

comment:19 Changed 5 years ago by tscrim

  • Branch set to public/groups/finite_reflection_groups-11187
  • Commit set to 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
1f3fe1eSome other fixes from changes to Sage in order to get Sage to start.

comment:20 Changed 5 years ago by git

  • Commit changed from 1f3fe1e0caa28a1cb6e2d17a61d94c89460e2bd4 to b60e61b6695fb1cde48d867390d50c32569fc2b0

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

bc4e068Merge branch 'public/groups/finite_reflection_groups-11187' of ssh://trac.sagemath.org:22/sage into 11187
b60e61btrac #11187 a few minor changes

comment:21 Changed 5 years ago by git

  • Commit changed from b60e61b6695fb1cde48d867390d50c32569fc2b0 to 1de360ff9e9c4db57e7de8b4ea45bbc4807e243c

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

1de360ftrac #11187 crystallo takes 2 l

comment:22 Changed 5 years ago by git

  • Commit changed from 1de360ff9e9c4db57e7de8b4ea45bbc4807e243c to deb13e3c50d259c1708aaa815300d94045a14b07

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

deb13e3trac #11187 trying to make doctest pass

comment:23 Changed 5 years ago by git

  • Commit changed from deb13e3c50d259c1708aaa815300d94045a14b07 to b6c1a6273f96f1b5d6103b3090eee3633b2f54b2

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

b6c1a62trac #11187 a few more corrections

comment:24 Changed 5 years ago by git

  • Commit changed from b6c1a6273f96f1b5d6103b3090eee3633b2f54b2 to becb1cab8eacec6e60661677fb2c1fcfe2429afe

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

becb1catrac #11187 a few more changes, removing duplicate functions

comment:25 Changed 5 years ago by git

  • Commit changed from becb1cab8eacec6e60661677fb2c1fcfe2429afe to e97998c532abc75eec6b18fecff6769601a1bade

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

da599f0Merge branch 'public/groups/finite_reflection_groups-11187' of ssh://trac.sagemath.org:22/sage into 11187
e97998ctrac #11187 more work, mostly minor

comment:26 Changed 5 years ago by git

  • Commit changed from e97998c532abc75eec6b18fecff6769601a1bade to 03111c8373909a6f4ac8e7c8b63625c74b4077a5

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

03111c8trac #11187 a little pyflakes cleanup

comment:27 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:28 Changed 5 years ago by git

  • Commit changed from 03111c8373909a6f4ac8e7c8b63625c74b4077a5 to 4a37eac384b296acde87af5bddbe39a4ba8addf0

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

6a0a3f3Merge branch 'public/groups/finite_reflection_groups-11187' of ssh://trac.sagemath.org:22/sage into 11187
4a37eactrac #11187 very tiny changes

comment:29 Changed 4 years ago by git

  • Commit changed from 4a37eac384b296acde87af5bddbe39a4ba8addf0 to 527cb502a4f3729472344a7fbb894502ddd03050

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

527cb50Merge branch 'public/groups/finite_reflection_groups-11187' into 6.6.b1

comment:30 Changed 4 years ago by vripoll

  • Cc vripoll added

comment:31 Changed 4 years ago by git

  • Commit changed from 527cb502a4f3729472344a7fbb894502ddd03050 to f2fbe0ad1c694936e6d15195055c261aadc17e1d

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

f2fbe0amerged in 6.8.beta2

comment:32 Changed 4 years ago by stumpc5

  • Branch changed from public/groups/finite_reflection_groups-11187 to u/stumpc5/11187

comment:33 Changed 4 years ago by git

  • Commit changed from f2fbe0ad1c694936e6d15195055c261aadc17e1d to ba7b5f092f41da9bbdb18455ecc5af2623dc42cc

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

ba7b5f0made Sage not crash on startup

comment:34 Changed 4 years ago by kdilks

  • Cc kdilks added

comment:35 Changed 4 years ago by stumpc5

  • Branch changed from u/stumpc5/11187 to u/stumpc5/11187-new

comment:36 Changed 4 years ago by git

  • Commit changed from ba7b5f092f41da9bbdb18455ecc5af2623dc42cc to 6ca40fa7459d11bf9c3564777d962a7493eb476c

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

a2f0f31first implementation of the iterator through magma elements
6ca40faMerge branch 't/18584/iterator_through_magma_elements' into u/stumpc5/11187-new

comment:37 Changed 4 years ago by git

  • Commit changed from 6ca40fa7459d11bf9c3564777d962a7493eb476c to 33f93c6061d90dd3849525f63aa7ffaa21049492

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

33f93c6removed the magma iterator

comment:38 Changed 4 years ago by git

  • Commit changed from 33f93c6061d90dd3849525f63aa7ffaa21049492 to 08a782138c922172d901834b691a80ec850f9bb2

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

08a7821fixed all doctests in src/sage/combinat/root_system/complex_reflection_group.py

comment:39 Changed 4 years ago by git

  • Commit changed from 08a782138c922172d901834b691a80ec850f9bb2 to 014fec229c73bb16da7825d24633c47076f22c4c

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

014fec2got the doctests of complex reflection group category pass

comment:40 Changed 4 years ago by git

  • Commit changed from 014fec229c73bb16da7825d24633c47076f22c4c to d3198f92cb72fd0ca67c4118ddcb00289143bd62

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

d3198f9fixing a few bugs on coxeter number and fundamental invariants

comment:41 Changed 4 years ago by git

  • Commit changed from d3198f92cb72fd0ca67c4118ddcb00289143bd62 to 8db1deeed4893580fd7c88d42d2fb9366a895bcf

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

e2736f6added another doctest
8db1deefirst version of adding functionality to coxeter group categories

comment:42 Changed 4 years ago by git

  • Commit changed from 8db1deeed4893580fd7c88d42d2fb9366a895bcf to 425ca38c084a51fa56c301153165269031fb5e0d

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

425ca38first version of coxeter groups from chevie with all tests passing!

comment:43 Changed 4 years ago by SimonKing

  • Cc SimonKing added

comment:44 Changed 4 years ago by SimonKing

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 4 years ago by SimonKing

To summarise the discussion Christian and I just had: I believe WellGenerated should be an axiom. Christian said that the general notion of well-generated is a bit tricky for infinite groups. Hence, he actually only wants to take into account well-generatedness 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 well-generated groups. Alternatively, it should be implemented as a nested-nested 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 well-generated. Such method would automatically be executed when running the test suite.

comment:46 Changed 4 years ago by SimonKing

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 sub-category of ComplexReflectionGroups.Finite(), we should probably remove its subcategory-methods Finite and Irreducible, since both axioms have already been defined in the super-category.

comment:47 Changed 4 years ago by SimonKing

The iterators currently cache the list of all elements of the group. Is that (always) a good idea?

comment:48 Changed 4 years ago by SimonKing

  • Branch changed from u/stumpc5/11187-new to u/SimonKing/11187-new

comment:49 Changed 4 years ago by SimonKing

  • Commit changed from 425ca38c084a51fa56c301153165269031fb5e0d to 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:

c0a036bAvoid creation of _reduced_word during iteration

comment:50 Changed 4 years ago by stumpc5

  • Branch changed from u/SimonKing/11187-new to u/stumpc5/11187-new

comment:51 Changed 4 years ago by chapoton

  • Commit changed from c0a036b2e2f01b1be6a7d004373fafe0c399e14d to 0639665e4f6ac2bee4a4cc9dfcaed5963d910155
  • Milestone changed from sage-6.4 to sage-6.8

New commits:

405b26cturned well-generated into an axiom
0639665fixed a but with Word(reduced_word) we worked on yesterday

comment:52 Changed 4 years ago by git

  • Commit changed from 0639665e4f6ac2bee4a4cc9dfcaed5963d910155 to 28f8abd2eb45700978592f1781a1f87e3c4d5e69

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

28f8abdadded a test for well generated

comment:53 Changed 4 years ago by SimonKing

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 4 years ago by chapoton

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 4 years ago by git

  • Commit changed from 28f8abd2eb45700978592f1781a1f87e3c4d5e69 to 697cd3f3077ef749bee642e0281b24fc66c3cbeb

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

3c79040added rational catalan numbers
697cd3freplaced asserts by raises and a few other fixes

comment:56 follow-up: Changed 4 years ago by stumpc5

@Simon and Nicolas: The category for complex reflection groups (with axioms finite/irreducible/well-generated) 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 to FiniteCoxeterGroups, its string repr changes in a not desired way. How should I treat that?

Thanks, Christian

comment:57 Changed 4 years ago by git

  • Commit changed from 697cd3f3077ef749bee642e0281b24fc66c3cbeb to 92ab198593eb2d772c48c4cfb80408d6da1daaa9

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

92ab198made the category of coxeter groups a subcategory of complex reflection groups

comment:58 in reply to: ↑ 56 Changed 4 years ago by tscrim

Replying to stumpc5:

@Simon and Nicolas: The category for complex reflection groups (with axioms finite/irreducible/well-generated) 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 to FiniteCoxeterGroups, 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 4 years ago by git

  • Commit changed from 92ab198593eb2d772c48c4cfb80408d6da1daaa9 to abc690c06e9f6b60bde44f689259d3787d5681ea

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

ec94ae3added a few more doctests for CoxeterGroupsChevie
1878996work in progress: new implementations are presumably correct, but existing bug discovered
5716df5Fixed according to Travis's suggestions
4404133Fixed according to Travis's suggestions, and now actually running I think
8fe1bb8Tab fixed
a6dcb8bmerged with now-resolved conflicts
c37a5efthings appear to work in the sense that all tests have passed.
1862325removed trailing whitespace.
1f29ae6reordered documentation, as requested by chapoton
abc690cmany doctests, and merged #18597

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

Hello,

  • once again: doctest continuation is now ....: and not ...
  • could you please put a positive review on the duplicate #9288 ?

comment:61 Changed 4 years ago by git

  • Commit changed from abc690c06e9f6b60bde44f689259d3787d5681ea to 742c0df5916cdd39d843553858f295df9a1ed18e

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

742c0dfreplaced '... ' by '....:'

comment:62 in reply to: ↑ 60 Changed 4 years ago by stumpc5

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 4 years ago by git

  • Commit changed from 742c0df5916cdd39d843553858f295df9a1ed18e to df37b7e9dbec9913ad652826e165e7f534ded0d3

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

ab914f5first version of the subword complexes as of 2015
cb3ca9cimprove the subword complex, in particular its documentation
f105602Merge branch 'u/stumpc5/11010' into u/stumpc5/11187-new
8618845playing, I hopefully can reorganize this again
ab4999fmore intelligent fix a la stump
df37b7eMerge branch 't/18610/bug__circular_descent_check_in_weylgroups' into u/stumpc5/11187-new

comment:64 Changed 4 years ago by stumpc5

  • Dependencies changed from #8327 #14137 to #8327 #14137 #18590

comment:65 Changed 4 years ago by stumpc5

  • Dependencies changed from #8327 #14137 #18590 to #8327 #14137 #18590 #18597

comment:66 Changed 4 years ago by git

  • Commit changed from df37b7e9dbec9913ad652826e165e7f534ded0d3 to e180c3133fb8c90f941b3f6f4be8a10e0bb15065

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

e180c31Revert "improve the subword complex, in particular its documentation"

comment:67 Changed 4 years ago by git

  • Commit changed from e180c3133fb8c90f941b3f6f4be8a10e0bb15065 to 450dadd370f2a8b1e6d05630d669742df3a1beef

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

450daddRevert "first version of the subword complexes as of 2015"

comment:68 Changed 4 years ago by stumpc5

I took out the subword complex stuff which is handled in #11010, and which is depending on this one.

comment:69 Changed 4 years ago by git

  • Commit changed from 450dadd370f2a8b1e6d05630d669742df3a1beef to e8e0d9ae9e5770b2088d6fb9ee6db412d21353a2

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

e8e0d9aadded two doctests

comment:70 Changed 4 years ago by stumpc5

  • Dependencies changed from #8327 #14137 #18590 #18597 to #8327 #14137 #18590 #18597 #18620

comment:71 Changed 4 years ago by stumpc5

  • Dependencies changed from #8327 #14137 #18590 #18597 #18620 to #18620

comment:72 Changed 4 years ago by git

  • Commit changed from e8e0d9ae9e5770b2088d6fb9ee6db412d21353a2 to 80b9a54c6f35f6204fd226df8e95df45b05435d3

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

e6de158added more doctests
80b9a54added more doctests, many more to come

comment:73 Changed 4 years ago by git

  • Commit changed from 80b9a54c6f35f6204fd226df8e95df45b05435d3 to d98ee4ecfcdc337ba74ec9a008dd58a17239cfde

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

a89a90fremoved trailing whitespace.
ce30fafreordered documentation, as requested by chapoton
21abe06replaced '... ' by '....:'
4121914improve the subword complex, in particular its documentation
b2b7f68more intelligent fix a la stump
d662af6Revert "improve the subword complex, in particular its documentation"
1ef2de7Revert "first version of the subword complexes as of 2015"
03b60e2added two doctests
2a34befadded more doctests
d98ee4eadded more doctests, many more to come

comment:74 Changed 4 years ago by git

  • Commit changed from d98ee4ecfcdc337ba74ec9a008dd58a17239cfde to 294d5f8f513b74bae5de22221b069a21df1ed6f4

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

294d5f8fixed a docstring

comment:75 Changed 4 years ago by git

  • Commit changed from 294d5f8f513b74bae5de22221b069a21df1ed6f4 to eddaef5ada286366d9dc8d1f5af329f915c9da76

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

2661cedlazy import of ComplexReflectionGroups
346532eold style doc replaced
894a671fixed some doctests for ComplexReflectionGroups
eddaef5more doctest for ComplexReflectionGroups, almost done there

comment:76 Changed 4 years ago by git

  • Commit changed from eddaef5ada286366d9dc8d1f5af329f915c9da76 to fb852f57be8fe8897ee2c2bad7b4847c865476e8

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

fb852f5added back the references to __init__

comment:77 Changed 4 years ago by git

  • Commit changed from fb852f57be8fe8897ee2c2bad7b4847c865476e8 to 3783ea23dc6a102eea745f59cd2a6b48fb87d5ca

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

3783ea2added some stuff that got lost during rebasing

comment:78 Changed 4 years ago by git

  • Commit changed from 3783ea23dc6a102eea745f59cd2a6b48fb87d5ca to 43f1dcedca38ef310502c66f4bc9147040d2972a

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

c07e76emoved from ComplexReflectionGroup/CoxeterGroupChevie to ReflectionGroup
f845089change in irreducible components
bbe138fMerge branch 't/18620/9a3e5cdacac40b505f7aad0a62b9e1bec76d8f3a' into u/stumpc5/11187-new
43f1dcereflection groups in the reference

comment:79 Changed 4 years ago by git

  • Commit changed from 43f1dcedca38ef310502c66f4bc9147040d2972a to 2f737ce0ee193dc7de8b89586dc3939f2686c161

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

2f737cegot the documentation run again as expected

comment:80 Changed 4 years ago by git

  • Commit changed from 2f737ce0ee193dc7de8b89586dc3939f2686c161 to 9df16f6d63f7107669e9229211f9520aeb39a7ad

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

9df16f6cleaned the documentation of the complex reflection group category

comment:81 Changed 4 years ago by git

  • Commit changed from 9df16f6d63f7107669e9229211f9520aeb39a7ad to bfd9aa684127059d97777ad55432772322a78ebc

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

bfd9aa6added some references to ComplexReflectionGroups

comment:82 Changed 4 years ago by git

  • Commit changed from bfd9aa684127059d97777ad55432772322a78ebc to e3b047bff2bd12f99a3fbea87f5417cd2c012745

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

e3b047badded some references to ComplexReflectionGroups

comment:83 Changed 4 years ago by stumpc5

  • Description modified (diff)
  • Keywords days 64.5 added

comment:84 Changed 4 years ago by git

  • Commit changed from e3b047bff2bd12f99a3fbea87f5417cd2c012745 to d6736681a50cfae4bcd58ad601af43860b5733e3

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

7c8cc37typo
dd3d515work on cartan and coxeter matrices
d673668removed a few patches that are not needed here

comment:85 Changed 4 years ago by git

  • Commit changed from d6736681a50cfae4bcd58ad601af43860b5733e3 to ef3cbe7303af54c2390e04404fb6a4c2623b9c3e

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

ef3cbe7fixed a docstring

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

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 by Return 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 in reply to: ↑ 86 Changed 4 years ago by stumpc5

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 4 years ago by chapoton

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 4 years ago by stumpc5

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 4 years ago by chapoton

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 4 years ago by git

  • Commit changed from ef3cbe7303af54c2390e04404fb6a4c2623b9c3e to 40d22b23447993f976e9680aec23d32c4f4c34f4

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

64fe437copyright and stripped trailing whitespaces
0a616e2minor movings of code
eee3af5moved reduced_word for real groups
40d22b2fixed todos, bad alignments, returns

comment:92 Changed 4 years ago by kdilks

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 4 years ago by kdilks

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 never-ending loop.

comment:94 Changed 4 years ago by jdemeyer

Replace

# optional (require chevie)

by

# optional - chevie

comment:95 Changed 4 years ago by jdemeyer

When adding new files, you should follow http://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files, in particular use the correct license statement.

comment:96 follow-up: Changed 4 years ago by stumpc5

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 in reply to: ↑ 96 ; follow-up: Changed 4 years ago by jdemeyer

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 in reply to: ↑ 97 ; follow-up: Changed 4 years ago by stumpc5

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.

Last edited 4 years ago by stumpc5 (previous) (diff)

comment:99 in reply to: ↑ 98 Changed 4 years ago by jdemeyer

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 non-trivial.

comment:100 Changed 4 years ago by stumpc5

  • Description modified (diff)
Note: See TracTickets for help on using tickets.