Opened 14 months ago
Closed 3 months ago
#32669 closed enhancement (fixed)
Adding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py
Reported by:  ghDennisJahn  Owned by:  

Priority:  major  Milestone:  sage9.8 
Component:  combinatorics  Keywords:  Coxeter groups, reflection groups, Bruhat order, Bruhat cones, 
Cc:  Travis Scrimshaw  Merged in:  
Authors:  Dennis Jahn  Reviewers:  Frédéric Chapoton, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  648e634 (Commits, GitHub, GitLab)  Commit:  648e6340df54ecdf456a4019a84dae2853661660 
Dependencies:  Stopgaps: 
Description
To a pair of elements x,y in a Coxeter group W one can associate two polyhedral cones. The 'upper bruhat cone' generated by all roots beta such that s_beta * x covers x and s_beta * x <= y and similarly the 'lower bruhat cone' generated by all beta sucht that y covers s_beta * y and x <= s_beta * y .
These cones were used in https://eudml.org/doc/174610 and https://arxiv.org/abs/2103.03715
Change History (59)
comment:1 Changed 14 months ago by
Summary:  Adding upper and lower Bruhat cones of M. Dyer to Coxeter groups → Adding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py 

comment:2 Changed 14 months ago by
Branch:  → u/ghDennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py 

comment:3 Changed 14 months ago by
Commit:  → 8774d2dc763fccfa8a4124d980001b079d117ca7 

comment:4 Changed 14 months ago by
Status:  new → needs_review 

comment:6 Changed 14 months ago by
The method reflection_to_positive_root() is used. As far as I saw neither WeylGroup? nor CoxeterGroup? has this method. WeylGroup? has furthermore no method to obtain roots roots.
comment:7 Changed 14 months ago by
Commit:  8774d2dc763fccfa8a4124d980001b079d117ca7 → da7e6b6287d4867de201ee282553e135ea1a7300 

Branch pushed to git repo; I updated commit sha1. New commits:
da7e6b6  Added zero vertex to the cone

comment:8 Changed 14 months ago by
Commit:  da7e6b6287d4867de201ee282553e135ea1a7300 → cce2fda76ee36bd3b358bd2cc5c615acd3ef18cd 

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

comment:9 Changed 14 months ago by
Commit:  cce2fda76ee36bd3b358bd2cc5c615acd3ef18cd → b9692a691663cc418e59d6e8b04c3c88c230a515 

Branch pushed to git repo; I updated commit sha1. New commits:
b9692a6  Changed 'Returns' to 'Return' in the doc

comment:10 Changed 14 months ago by
in Weyl Groups, there is
src/sage/categories/weyl_groups.py: def reflection_to_root
comment:11 Changed 14 months ago by
I see. Then writing a similar method for Weyl Groups should be possible using reflection_to_root() and then to_ambient()
comment:12 Changed 14 months ago by
Commit:  b9692a691663cc418e59d6e8b04c3c88c230a515 → 898ddd7165e2e7c4edeb690beaf05e8322ff9c3a 

Branch pushed to git repo; I updated commit sha1. New commits:
898ddd7  style changes as mentioned by ghkliem in 32681

comment:13 Changed 14 months ago by
Commit:  898ddd7165e2e7c4edeb690beaf05e8322ff9c3a → f7a19db8d6ce5c5095454fee0823e0ff693cb99d 

Branch pushed to git repo; I updated commit sha1. New commits:
f7a19db  its cdd not ccd

comment:14 followup: 19 Changed 13 months ago by
Thank you for your contribuation. I have a rather long list of comments. Most of them are minor issues I think.
A green bot unfortunatly only means that you added the optional tags correctly, as the bots don't have gap3 installed.
 You should probably add your references from the ticket description. As pointed in https://doc.sagemath.org/html/en/developer/coding_basics.html#pythoncodestyle
at REFERENCES
and ALGORITHM
.

wi = self.apply_simple_reflection(i, side = 'right') return [(u.apply_simple_reflection(i, side='right'),r.apply_conjugation_by_simple_reflection(i)) for u,r in wi.bruhat_lower_covers_reflections() if not u.has_descent(i, side='right')] + [(wi, self.parent().simple_reflection(i))]
Those lines need some work.
side='right
is a keyword and should therefore not contain spaces. The second line is way to long and should be wrapped according to PEP8. We are not ultra stright with 79 characters, but it is still good to keep it at that. Also it is impossible to understand what this line does like this. So I would propose:wi = self.apply_simple_reflection(i, side='right') return [(u.apply_simple_reflection(i, side='right'), r.apply_conjugation_by_simple_reflection(i)) for u, r in wi.bruhat_lower_covers_reflections() if not u.has_descent(i, side='right')] + [ (wi, self.parent().simple_reflection(i))]Yes, I know that you did not introduce this ultralong line, but it is still good to fix it, when touching.
 From the ticket I gather that your changes in
coxeter_group.py
are motivated by a bug. Can you please add a test toCoxeterGroups
that illustrates what has been fixed now. Something:TESTS: Check bug discovered in :trac:`32669` is fixed: sage: 2 + 2 4
 The description on
bruhat_cone
is hard to read. The first line should just be a brief sentence describing what the function does. No need to contain all definitions.  This should be reworked:
I don't know what it means. What are those things called:
+ For ``side`` = ``'upper'``: ``s_beta`` ``x`` covers ``x`` and ``x`` <= ``s_beta`` ``x`` <= ``y``. + + For ``side`` = ``'lower'``: ``y`` covers ``s_beta`` ``y`` and ``x`` <= ``s_beta`` ``y`` <= ``y``.
Upper Bruhat cone
andLower Bruhat cone
? If you just call them by names, that already makes the start easier to read. You know you can also use latex in the docstring. Maybe this makes it easier to read:
https://doc.sagemath.org/html/en/developer/coding_basics.html#sectionlatextypeset
 The input
x
andy
needs to be explained.  Your doctests do not pass:
File "src/sage/combinat/root_system/reflection_group_real.py", line 732, in sage.combinat.root_system.reflection_group_real.RealReflectionGroup.bruhat_cone Failed example: W.bruhat_cone(x, y) # optional  gap3 Expected: A 2dimensional polyhedron in ZZ^2 defined as the convex hull of 1 vertex and 2 rays Got: A 2dimensional polyhedron in QQ^2 defined as the convex hull of 1 vertex and 2 rays ********************************************************************** File "src/sage/combinat/root_system/reflection_group_real.py", line 738, in sage.combinat.root_system.reflection_group_real.RealReflectionGroup.bruhat_cone Failed example: W.bruhat_cone(x, y, side = 'lower') # optional  gap3 Expected: A 6dimensional polyhedron in ZZ^6 defined as the convex hull of 1 vertex and 6 rays Got: A 6dimensional polyhedron in QQ^6 defined as the convex hull of 1 vertex and 6 rays
There are also other failing doctests discovered, because people apparently do not test gap3
at all:
File "src/sage/combinat/root_system/reflection_group_element.pyx", line 72, in sage.combinat.root_system.reflection_group_element.ComplexReflectionGroupElement.__hash__ Failed example: WB_hash.intersection(WC_hash) # optional  gap3 Expected: set() Got: {9126606217563690753, 9111617679337390865, 9021783181305004801, 9008457096590967825, 8825287278071369473, 8737678173820754961, 8665277549911544081, 8446633060280535041, 8323654867644563729, 8124071536798024977, 8116295769124982033, 8108062643194285329, 8049278353572089345, 8046023783577836817, 8045847864374923025, 8042003968497354753, 7957288799848911889, ...
However, you did not cause this I think. If we don't fix them here (which definitely needs not to happen), which should open a ticket for them. Likewise
src/sage/categories/coxeter_groups.py
does not pass:$ sage t long optional=build,debian,dochtml,e_antic,normaliz,pip,pynormaliz,sage,sage.geometry.polyhedron,sage.rings.real_double,sage_spkg,gap3 src/sage/categories/coxeter_groups.py too many failed tests, not using stored timings Running doctests with ID 2021112212293105e09ddd. Git branch: test_32669 Using optional=build,debian,dochtml,e_antic,gap3,normaliz,pip,pynormaliz,sage,sage.geometry.polyhedron,sage.rings.real_double,sage_spkg Doctesting 1 file. sage t long randomseed=289954757401290987674537126910226284546 src/sage/categories/coxeter_groups.py ********************************************************************** File "src/sage/categories/coxeter_groups.py", line 240, in sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_group_as_finitely_presented_group Failed example: W.braid_group_as_finitely_presented_group() # optional  gap3 Exception raised: Traceback (most recent call last): File "sage/misc/cachefunc.pyx", line 996, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5998) return self.cache[k] File "sage/misc/weak_dict.pyx", line 704, in sage.misc.weak_dict.WeakValueDictionary.__getitem__ (build/cythonized/sage/misc/weak_dict.c:3752) raise KeyError(k) KeyError: ((<class 'sage.combinat.root_system.type_relabel.CartanType'>, ['B', 3], Finite family {1: 5, 2: 'BB', 3: 'AA'}), ()) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 694, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 1088, in compile_and_execute exec(compiled, globs) File "<doctest sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_group_as_finitely_presented_group[5]>", line 1, in <module> W.braid_group_as_finitely_presented_group() # optional  gap3 File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 251, in braid_group_as_finitely_presented_group rels = self.braid_relations() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_complex.py", line 1392, in braid_relations return super(ComplexReflectionGroup,self).braid_relations() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 215, in braid_relations M = self.coxeter_matrix() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 639, in coxeter_matrix return self.cartan_type().coxeter_matrix() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 430, in cartan_type return C.relabel(CG.is_isomorphic(G, edge_labels=True, certificate=True)[1]) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/cartan_type.py", line 1236, in relabel return type_relabel.CartanType(self, relabelling) File "sage/misc/classcall_metaclass.pyx", line 320, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1779) return cls.classcall(cls, *args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 57, in __classcall__ return super(CartanType, cls).__classcall__(cls, type, relabelling) File "sage/misc/cachefunc.pyx", line 1001, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:6126) w = self.f(*args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/structure/unique_representation.py", line 1007, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 482, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:2243) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 179, in __init__ self._index_set = tuple(sorted(relabelling[i] for i in type.index_set())) TypeError: '<' not supported between instances of 'str' and 'int' ********************************************************************** File "src/sage/categories/coxeter_groups.py", line 288, in sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_orbit Failed example: W.braid_orbit(w.reduced_word()) # optional  gap3 Exception raised: Traceback (most recent call last): File "sage/misc/cachefunc.pyx", line 996, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5998) return self.cache[k] File "sage/misc/weak_dict.pyx", line 704, in sage.misc.weak_dict.WeakValueDictionary.__getitem__ (build/cythonized/sage/misc/weak_dict.c:3752) raise KeyError(k) KeyError: ((<class 'sage.combinat.root_system.type_relabel.CartanType'>, ['A', 3], Finite family {1: 'AA', 2: 'BB', 3: 5}), ()) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 694, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 1088, in compile_and_execute exec(compiled, globs) File "<doctest sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_orbit[8]>", line 1, in <module> W.braid_orbit(w.reduced_word()) # optional  gap3 File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 318, in braid_orbit braid_rels = self.braid_relations() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_complex.py", line 1392, in braid_relations return super(ComplexReflectionGroup,self).braid_relations() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 215, in braid_relations M = self.coxeter_matrix() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 639, in coxeter_matrix return self.cartan_type().coxeter_matrix() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 430, in cartan_type return C.relabel(CG.is_isomorphic(G, edge_labels=True, certificate=True)[1]) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/cartan_type.py", line 1236, in relabel return type_relabel.CartanType(self, relabelling) File "sage/misc/classcall_metaclass.pyx", line 320, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1779) return cls.classcall(cls, *args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 57, in __classcall__ return super(CartanType, cls).__classcall__(cls, type, relabelling) File "sage/misc/cachefunc.pyx", line 1001, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:6126) w = self.f(*args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/structure/unique_representation.py", line 1007, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 482, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:2243) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 179, in __init__ self._index_set = tuple(sorted(relabelling[i] for i in type.index_set())) TypeError: '<' not supported between instances of 'int' and 'str' ********************************************************************** File "src/sage/categories/coxeter_groups.py", line 1538, in sage.categories.coxeter_groups.CoxeterGroups.ElementMethods.reduced_words Failed example: w.reduced_words() # optional  gap3 Exception raised: Traceback (most recent call last): File "sage/misc/cachefunc.pyx", line 996, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5998) return self.cache[k] File "sage/misc/weak_dict.pyx", line 704, in sage.misc.weak_dict.WeakValueDictionary.__getitem__ (build/cythonized/sage/misc/weak_dict.c:3752) raise KeyError(k) KeyError: ((<class 'sage.combinat.root_system.type_relabel.CartanType'>, ['A', 3], Finite family {1: 'AA', 2: 'BB', 3: 5}), ()) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 694, in _run self.compile_and_execute(example, compiler, test.globs) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/doctest/forker.py", line 1088, in compile_and_execute exec(compiled, globs) File "<doctest sage.categories.coxeter_groups.CoxeterGroups.ElementMethods.reduced_words[9]>", line 1, in <module> w.reduced_words() # optional  gap3 File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 1566, in reduced_words return self.parent().braid_orbit(self.reduced_word()) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 318, in braid_orbit braid_rels = self.braid_relations() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_complex.py", line 1392, in braid_relations return super(ComplexReflectionGroup,self).braid_relations() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/categories/coxeter_groups.py", line 215, in braid_relations M = self.coxeter_matrix() File "sage/misc/cachefunc.pyx", line 2310, in sage.misc.cachefunc.CachedMethodCallerNoArgs.__call__ (build/cythonized/sage/misc/cachefunc.c:12957) self.cache = f(self._instance) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 639, in coxeter_matrix return self.cartan_type().coxeter_matrix() File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/reflection_group_real.py", line 430, in cartan_type return C.relabel(CG.is_isomorphic(G, edge_labels=True, certificate=True)[1]) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/cartan_type.py", line 1236, in relabel return type_relabel.CartanType(self, relabelling) File "sage/misc/classcall_metaclass.pyx", line 320, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1779) return cls.classcall(cls, *args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 57, in __classcall__ return super(CartanType, cls).__classcall__(cls, type, relabelling) File "sage/misc/cachefunc.pyx", line 1001, in sage.misc.cachefunc.CachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:6126) w = self.f(*args, **kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/structure/unique_representation.py", line 1007, in __classcall__ instance = typecall(cls, *args, **options) File "sage/misc/classcall_metaclass.pyx", line 482, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:2243) return (<PyTypeObject*>type).tp_call(cls, args, kwds) File "/srv/public/kliem/sage/local/var/lib/sage/venvpython3.7/lib/python3.7/sitepackages/sage/combinat/root_system/type_relabel.py", line 179, in __init__ self._index_set = tuple(sorted(relabelling[i] for i in type.index_set())) TypeError: '<' not supported between instances of 'int' and 'str' ********************************************************************** 3 items had failures: 1 of 11 in sage.categories.coxeter_groups.CoxeterGroups.ElementMethods.reduced_words 1 of 7 in sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_group_as_finitely_presented_group 1 of 10 in sage.categories.coxeter_groups.CoxeterGroups.ParentMethods.braid_orbit [504 tests, 3 failures, 5.61 s]
 It would be good to have doctests that illustrate some actual properties. E.g. you could illustrate the definition.
 In your actual code there are also some very long lines. They could also be wrapped. E.g.:
 roots = [self.reflection_to_positive_root(x*r*x.inverse()) for z, r in x.bruhat_upper_covers_reflections() if z.bruhat_le(y)] + roots = [self.reflection_to_positive_root(x * r * x.inverse()) + for z, r in x.bruhat_upper_covers_reflections() + if z.bruhat_le(y)]
 After the
ValueError
there could be an empty line. Likewise an empty line would make sense afterfrom sage.rings.qqbar import AA as base_ring
, because this is when theif ... else
is over.
comment:15 Changed 13 months ago by
Commit:  f7a19db8d6ce5c5095454fee0823e0ff693cb99d → 37ff44a2642c40c90f060d8f8bc4784fe0e9231c 

Branch pushed to git repo; I updated commit sha1. New commits:
37ff44a  added test to check bug in coxeter_groups.py in bruhat_lower_covers_reflections() woth implementation 'permutation' and cleaned up code according to PEP8

comment:16 Changed 13 months ago by
Commit:  37ff44a2642c40c90f060d8f8bc4784fe0e9231c → c816e5543d2e6570a37554d6cca27e380e72d524 

Branch pushed to git repo; I updated commit sha1. New commits:
c816e55  syntax for doc highlighting

comment:17 Changed 13 months ago by
Commit:  c816e5543d2e6570a37554d6cca27e380e72d524 → 90393b23d1f05be6cdfe6f0d23329c503032dea7 

Branch pushed to git repo; I updated commit sha1. New commits:
90393b2  better description for bruhat cone, wrapped and empty lines, corrected doctests

comment:18 Changed 13 months ago by
Commit:  90393b23d1f05be6cdfe6f0d23329c503032dea7 → 185407eaee2571f27522c365de29db698ef61237 

Branch pushed to git repo; I updated commit sha1. New commits:
185407e  added references

comment:19 Changed 13 months ago by
Replying to ghkliem:
Thank you very much for your detailed feedback.
 You should probably add your references from the ticket description.
I added the references.
 Those lines need some work.
 The second line is way to long and should be wrapped according to PEP8.
 Also it is impossible to understand what this line does like this.
I changed the lines according to your suggestions.
 From the ticket I gather that your changes in
coxeter_groups.py
are motivated by a bug. Can you please add a test toCoxeterGroups
that illustrates what has been fixed now.
I added a test. The bug did only occur for the permutation implementation, so the test is only for this implementation.
 The description on
bruhat_cone
is hard to read.
There now is a new description using latex but I'm not sure if this description would be better in an ALGORITHM
section.
 Your doctests do not pass:
They should pass now.
 There are also other failing doctests discovered, because people apparently do not test
gap3
at all.
I would prefer opening a new ticket for this.
 In your actual code there are also some very long lines.
I wrapped them and added the suggested empty lines.
comment:20 followup: 22 Changed 12 months ago by
Tests:
should be TESTS:
for arxiv, use the arxiv role, namely :arxiv:`2103.03715`
the new raise
must have a doctest
The else
here is superfluous :
+ else: + if backend == 'cdd':
Removing it would allow to indent the code less
comment:21 Changed 12 months ago by
Commit:  185407eaee2571f27522c365de29db698ef61237 → b950d056726bf23332fa9abcbaa2b4c5675be931 

comment:22 Changed 12 months ago by
Replying to chapoton:
Tests:
should beTESTS:
for arxiv, use the arxiv role, namely
:arxiv:`2103.03715`
the new
raise
must have a doctestThe
else
here is superfluous :+ else: + if backend == 'cdd':Removing it would allow to indent the code less
Thanks!
I changed/added the parts according to your comment.
The local doctests for reflection_group_real.py
including gap3
passed.
comment:23 followup: 25 Changed 12 months ago by
the new tests for raise are not indented correctly
comment:24 Changed 12 months ago by
Commit:  b950d056726bf23332fa9abcbaa2b4c5675be931 → 4486d0239a842810ea5f2c502e5085f79d57a116 

Branch pushed to git repo; I updated commit sha1. New commits:
4486d02  corrected the indentation

comment:25 Changed 12 months ago by
Replying to chapoton:
the new tests for raise are not indented correctly
You are correct, I completely missed that.
comment:26 Changed 12 months ago by
Commit:  4486d0239a842810ea5f2c502e5085f79d57a116 → abfb07b64fd1e16a11c1708ff5a5b93893ba30e2 

Branch pushed to git repo; I updated commit sha1. New commits:
abfb07b  moved around some spaces

comment:28 Changed 12 months ago by
Commit:  abfb07b64fd1e16a11c1708ff5a5b93893ba30e2 → 8fd159d9db6c0048174210a840799350ce729438 

Branch pushed to git repo; I updated commit sha1. New commits:
8fd159d  typo correction

comment:29 Changed 12 months ago by
Commit:  8fd159d9db6c0048174210a840799350ce729438 → 1e3d994aa6567094255467d3eb8157feb6c69ebf 

Branch pushed to git repo; I updated commit sha1. New commits:
1e3d994  extra colon for indented block

comment:30 Changed 12 months ago by
Milestone:  sage9.5 → sage9.6 

comment:31 Changed 9 months ago by
Status:  needs_review → needs_work 

red branch, so needs a rebase => needs_work status
comment:32 Changed 9 months ago by
Commit:  1e3d994aa6567094255467d3eb8157feb6c69ebf → 9181ed30bd3bb1301f072e1e78ef9b70f9a9cd1b 

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

43846a6  changed Tests to TESTS

82612d6  use the arxiv role

37400d8  removed the superfluous else

728a1a3  added a doctest for the raise

5f1f644  corrected the indentation

17e6fca  moved around some spaces

04157ae  typo correction

40efbb0  extra colon for indented block

9181ed3  Merge branch 'u/ghDennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py' of trac.sagemath.org:sage into t/32669/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py

comment:33 Changed 9 months ago by
Commit:  9181ed30bd3bb1301f072e1e78ef9b70f9a9cd1b → 1fe0f7240c8719e67a5c8fa8cbe393052aa1066e 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
deda01c  syntax for doc highlighting

4c1d6c9  better description for bruhat cone, wrapped and empty lines, corrected doctests

5cb51a0  added references

3db92aa  changed Tests to TESTS

3805d4e  use the arxiv role

6966092  corrected the indentation

87e8d8f  moved around some spaces

13d749d  typo correction

f190607  extra colon for indented block

1fe0f72  Merge branch 'u/ghDennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py' of trac.sagemath.org:sage into t/32669/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py

comment:34 Changed 9 months ago by
Status:  needs_work → needs_review 

I did a rebase but I'm not really sure if I did it correctly. There are three doctests failing but they are on methods I didn't touch.
Setting it back to needs_review for comments and (maybe) a review.
comment:35 Changed 8 months ago by
Milestone:  sage9.6 → sage9.7 

comment:36 followup: 40 Changed 8 months ago by
Cc:  Travis Scrimshaw added 

Your branch is a little mess, with too many commits. I would suggest to start a new branch afresh. Please just copypaste your changes atop the latest develop, so that there is only one commit.
I am also not so sure about the "bug" in Coxeter groups. Travis, any opinion ?
And you should rather add doctests not depending on the gap3 package too.
comment:37 Changed 8 months ago by
It may be a good idea to add an alias
reflection_to_positive_root = reflection_to_root
in
src/sage/categories/weyl_groups.py
comment:38 Changed 8 months ago by
I also think we should not use gap3
for the doctests. There are plenty of other implementations available.
The “bug” should be on a separate ticket, but I don’t know if it is a bug. Nevertheless, it is likely an improvement because we should be explicit about the side (although it should be consistent).
comment:39 Changed 8 months ago by
Commit:  1fe0f7240c8719e67a5c8fa8cbe393052aa1066e → 9ed53c18f8356582f2fa8927d5ed7f8cb5d7abf9 

Branch pushed to git repo; I updated commit sha1. New commits:
8a60078  added flag to bruhat_lower_covers_reflections() similar to bruhat_lower_covers and bruhat_upper_covers, this fixed a bug only appearing while using the permutation implementation  see doctest

0ef52be  added the method bruhat_cone including references

9ed53c1  Merge branch 'u/ghDennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py' of trac.sagemath.org:sage into t/32669/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py

comment:40 followup: 41 Changed 8 months ago by
Replying to chapoton:
Your branch is a little mess, with too many commits. I would suggest to start a new branch afresh. Please just copypaste your changes atop the latest develop, so that there is only one commit.
I hope this worked.
I am also not so sure about the "bug" in Coxeter groups. Travis, any opinion ?
The bug only appeared using the permutation implementation. If you run the new doctest with the old function you should see the problem.
And you should rather add doctests not depending on the gap3 package too.
I'm confused about that. The implementation of ReflectionGroup? in reflection_group_real.py heavily depends on gap3 and cannot be used without it. Why should there be doctests without gap3? Is this even possible?
comment:41 followup: 42 Changed 8 months ago by
Replying to ghDennisJahn:
Replying to chapoton:
Your branch is a little mess, with too many commits. I would suggest to start a new branch afresh. Please just copypaste your changes atop the latest develop, so that there is only one commit.
I hope this worked.
No. If you click on the word "Commits" here above, you will see that your branch is still made of very many commits. Maybe I was not clear enough in my suggestion ? Save outside of the the git repository of sage the current status (in your branch) of the 2 files that you have modified. Then use git to start a new branch on top of develop. Then copy back the modified files and commit. Then push here under a new branch name.
And you should rather add doctests not depending on the gap3 package too.
I'm confused about that. The implementation of ReflectionGroup? in reflection_group_real.py heavily depends on gap3 and cannot be used without it. Why should there be doctests without gap3? Is this even possible?
Gap3 itself is no longer maintained, and it's rather miraculous that it still works and that one can use it for reflection groups in sage. Using comment:37, your code should work on Weyl groups and be doctested for them.
comment:42 followup: 45 Changed 8 months ago by
Replying to chapoton:
No. If you click on the word "Commits" here above, you will see that your branch is still made of very many commits. Maybe I was not clear enough in my suggestion ? Save outside of the the git repository of sage the current status (in your branch) of the 2 files that you have modified. Then use git to start a new branch on top of develop. Then copy back the modified files and commit. Then push here under a new branch name.
I used 'git reset hard develop' locally and pushed my modifications after that but that wasn't enough then. I'll try something else.
Gap3 itself is no longer maintained, and it's rather miraculous that it still works and that one can use it for reflection groups in sage. Using comment:37, your code should work on Weyl groups and be doctested for them.
The method bruhat_cone is already implemented for Weyl groups. That was done in #32710. The purpose of this ticket is to implement it for reflection groups to also include noncrystallographic coxeter groups and further use the implementation for subword complexes in #32681.
comment:43 Changed 8 months ago by
Commit:  9ed53c18f8356582f2fa8927d5ed7f8cb5d7abf9 → 57fd20c3f14b9abf71b16039048855eef4b3f95f 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
comment:44 Changed 8 months ago by
Commit:  57fd20c3f14b9abf71b16039048855eef4b3f95f → 84b5726017c226182cf917441afc347f22dc1b94 

Branch pushed to git repo; I updated commit sha1. New commits:
d8ccc5a  added flag to bruhat_lower_covers_reflections() similar to bruhat_lower_covers and bruhat_upper_covers, this fixed a bug only appearing while using the permutation implementation  see doctest

84b5726  added the method bruhat_cone including references

comment:45 Changed 8 months ago by
Replying to ghDennisJahn:
I used 'git reset hard develop' locally and pushed my modifications after that but that wasn't enough then. I'll try something else.
Now it should have worked. There are only two entries in the list of commits, one for each modified file.
comment:46 followup: 47 Changed 8 months ago by
thanks.
Now, I see a lot of code duplication with the existing implementation in Weyl groups found in
src/sage/categories/weyl_groups.py
In my opinion, it is desirable and should be possible to have only one common implementation.
comment:47 Changed 8 months ago by
Replying to chapoton:
thanks.
Now, I see a lot of code duplication with the existing implementation in Weyl groups found in
src/sage/categories/weyl_groups.pyIn my opinion, it is desirable and should be possible to have only one common implementation.
I would agree but I think we had this discussion already. I can' find it anymore though.
The only parent/super category in common is coxeter groups and there is no implementation of roots or the cartan matrix in there. Furthermore the implementation of roots in Weyl groups is very different from the one in reflection groups. In Weyl groups their embedding depends on the degree of the group (A4 having 5, E6 and E8 having 8), while in reflection groups it depends on the rank.
I currently have no idea how to combine these worlds.
comment:48 followup: 49 Changed 8 months ago by
This is still done at the category level, so there should be some other implementation not relying on gap3
, right?
I don’t understand the point you are making about the implementation of the roots. The data is welldefined and the root systems are isomorphic. I agree with @chapoton that we should have one common code that is agnostic to the choice of implementation. If things are not melding together properly, then we need to look at the code and unify it somehow.
Then there is an issue with the permutation implementation about either its internal consistency or with the generic implementation that needs to be fixed. This would sweep the issue under the rug.
comment:49 Changed 8 months ago by
Replying to tscrim:
This is still done at the category level, so there should be some other implementation not relying on
gap3
, right?
The implementation for Weyl groups is done in
src/sage/categories/weyl_groups.py
at the category level, yes. The implementation for this ticket is done in
src/sage/combinat/root_system/reflection_group_real.py
what is, if I understand correctly, not at the catagory level.
Furthermore, I think, the only super category in common is CoxeterGroups?. And for reflection groups this seems to be the case only sometimes? What I mean is that, depending on the input data, sometimes a reflection group is initialized as Coxeter group, but not always.
So my question/problem is: Where should an implementation of 'Bruhat cones' be done such that both, Weyl groups and reflection groups, can use it? Wherever that is should be an existing implementation of roots that is compatible with the implementations used for Weyl groups and reflection groups.
Then there is an issue with the permutation implementation about either its internal consistency or with the generic implementation that needs to be fixed. This would sweep the issue under the rug.
I did not dive fully into this function. It wasn't working and all the other, similar functions used the side='right' flag. So I tried that and then it worked...
comment:50 Changed 3 months ago by
Dear Travis and Frederic, @tscrim, @chapoton:
Dennis is about to finish his thesis and I would like this code to go in! I do not understand your points that this should go into the category level because neither Coxeter groups (finite or not) or complex reflection groups there come with a root system attached. This code heavily uses root systems, so this is needed.
WeylGroups have this at the category level, that's why it worked for these.
Best, Christian
comment:51 Changed 3 months ago by
Reviewers:  → Frédéric Chapoton 

Status:  needs_review → positive_review 
oh, well. Let's move on.
Let me note that to still depend on gap3 in 2022 is a bit sad.
comment:52 Changed 3 months ago by
Thanks Frederic!
Let me note that to still depend on gap3 in 2022 is a bit sad.
I agree, but Jean Michel is porting it to julia (https://github.com/jmichel7/Gapjm.jl). Once this is done, we can move there!
comment:54 Changed 3 months ago by
Status:  positive_review → needs_work 

I just have some little doc things though before merging it in:
Please move the references to the master reference file and roughly follow the format there.
  ``x``  an element in the group `W`    ``y``  an element in the group `W`    ``side`` (default: ``'upper'``)  must be one of the following: +  ``x``  an element in the group `W` +  ``y`` — an element in the group `W` +  ``side`` — (default: ``'upper'``) must be one of the following:
comment:55 Changed 3 months ago by
Commit:  84b5726017c226182cf917441afc347f22dc1b94 → 648e6340df54ecdf456a4019a84dae2853661660 

comment:56 Changed 3 months ago by
Status:  needs_work → needs_review 

comment:57 Changed 3 months ago by
Reviewers:  Frédéric Chapoton → Frédéric Chapoton, Travis Scrimshaw 

Status:  needs_review → positive_review 
Thank you.
comment:58 Changed 3 months ago by
Milestone:  sage9.7 → sage9.8 

comment:59 Changed 3 months ago by
Branch:  u/ghDennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py → 648e6340df54ecdf456a4019a84dae2853661660 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
added missing flag side = 'right' to some methods used in bruhat_lower_covers_reflections()
merged two methods together to one method with a flag, rearranged stuff for the docmentation