Adding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py #32669

DennisJahn · 2021-10-11T13:05:22Z

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

CC: @tscrim

Component: combinatorics

Keywords: Coxeter groups, reflection groups, Bruhat order, Bruhat cones

Author: Dennis Jahn

Branch/Commit: 648e634

Reviewer: Frédéric Chapoton, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32669

The text was updated successfully, but these errors were encountered:

DennisJahn · 2021-10-12T13:22:19Z

Branch: u/gh-DennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py

sagetrac-git · 2021-10-13T09:43:01Z

Commit: 8774d2d

sagetrac-git · 2021-10-13T09:43:01Z

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

`41b85a0`	`added missing flag side = 'right' to some methods used in bruhat_lower_covers_reflections()`
`8774d2d`	`merged two methods together to one method with a flag, rearranged stuff for the docmentation`

fchapoton · 2021-10-13T15:34:20Z

comment:5

would examples with WeylGroup or CoxeterGroup work ?

DennisJahn · 2021-10-14T07:08:03Z

comment:6

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.

sagetrac-git · 2021-10-14T07:40:22Z

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

`da7e6b6`	`Added zero vertex to the cone`

sagetrac-git · 2021-10-14T07:40:22Z

Changed commit from 8774d2d to da7e6b6

sagetrac-git · 2021-10-14T13:36:16Z

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

`cce2fda`	`doc syntax`

sagetrac-git · 2021-10-14T13:36:16Z

Changed commit from da7e6b6 to cce2fda

sagetrac-git · 2021-10-15T07:19:45Z

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

`b9692a6`	`Changed 'Returns' to 'Return' in the doc`

sagetrac-git · 2021-10-15T07:19:45Z

Changed commit from cce2fda to b9692a6

fchapoton · 2021-10-15T07:32:55Z

comment:10

in Weyl Groups, there is

src/sage/categories/weyl_groups.py:        def reflection_to_root

DennisJahn · 2021-10-15T08:03:21Z

comment:11

I see. Then writing a similar method for Weyl Groups should be possible using reflection_to_root() and then to_ambient()

sagetrac-git · 2021-10-25T13:38:39Z

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

`898ddd7`	`style changes as mentioned by gh-kliem in 32681`

sagetrac-git · 2021-10-25T13:38:39Z

Changed commit from b9692a6 to 898ddd7

sagetrac-git · 2021-10-25T14:30:08Z

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

`f7a19db`	`its cdd not ccd`

sagetrac-git · 2021-10-25T14:30:08Z

Changed commit from 898ddd7 to f7a19db

kliem · 2021-11-22T11:43:05Z

comment:14

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#python-code-style
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 ultra-long 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 to CoxeterGroups 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:

+        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``.

I don't know what it means. What are those things called: Upper Bruhat cone and Lower 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#section-latex-typeset

The input x and y 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 2-dimensional polyhedron in ZZ^2 defined as the convex hull of 1 vertex and 2 rays
Got:
    A 2-dimensional 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 6-dimensional polyhedron in ZZ^6 defined as the convex hull of 1 vertex and 6 rays
Got:
    A 6-dimensional 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 2021-11-22-12-29-31-05e09ddd.
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 --random-seed=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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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/venv-python3.7/lib/python3.7/site-packages/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 after from sage.rings.qqbar import AA as base_ring, because this is when the if ... else is over.

sagetrac-git · 2021-11-23T12:29:35Z

Changed commit from f7a19db to 37ff44a

sagetrac-git · 2021-11-23T12:29:35Z

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`

sagetrac-git · 2021-11-23T12:54:55Z

Changed commit from 37ff44a to c816e55

sagetrac-git · 2021-11-23T12:54:55Z

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

`c816e55`	`syntax for doc highlighting`

sagetrac-git · 2021-11-24T12:28:55Z

Changed commit from c816e55 to 90393b2

sagetrac-git · 2021-11-24T12:28:55Z

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

`90393b2`	`better description for bruhat cone, wrapped and empty lines, corrected doctests`

DennisJahn · 2022-04-14T07:35:18Z

comment:45

Replying to @DennisJahn:

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.

fchapoton · 2022-04-14T08:13:06Z

comment:46

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.

DennisJahn · 2022-04-14T09:01:49Z

comment:47

Replying to @fchapoton:

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.

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.

tscrim · 2022-04-16T17:40:28Z

comment:48

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 well-defined and the root systems are isomorphic. I agree with @fchapoton 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.

DennisJahn · 2022-04-19T08:12:32Z

comment:49

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

stumpc5 · 2022-09-12T10:45:43Z

comment:50

Dear Travis and Frederic, @tscrim, @fchapoton:

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

fchapoton · 2022-09-12T11:56:45Z

Reviewer: Frédéric Chapoton

fchapoton · 2022-09-12T11:56:45Z

comment:51

oh, well. Let's move on.

Let me note that to still depend on gap3 in 2022 is a bit sad.

stumpc5 · 2022-09-12T12:05:23Z

comment:52

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!

fchapoton · 2022-09-12T12:53:27Z

comment:53

Well, this does not sound like really good news to me.

tscrim · 2022-09-12T13:15:23Z

comment:54

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:

sagetrac-git · 2022-09-13T07:50:05Z

Changed commit from 84b5726 to 648e634

sagetrac-git · 2022-09-13T07:50:05Z

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

`70ba7b5`	`added Dy1994 and JS2021 to master reference file`
`648e634`	`moved references and deleted empty lines`

tscrim · 2022-09-13T07:59:08Z

comment:57

Thank you.

tscrim · 2022-09-13T07:59:08Z

Changed reviewer from Frédéric Chapoton to Frédéric Chapoton, Travis Scrimshaw

vbraun · 2022-09-20T20:23:16Z

Changed branch from u/gh-DennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py to 648e634

DennisJahn added this to the sage-9.5 milestone Oct 11, 2021

DennisJahn added c: combinatorics labels Oct 11, 2021

DennisJahn changed the title ~~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 Oct 12, 2021

DennisJahn added the s: needs review label Oct 13, 2021

fchapoton added s: positive review and removed s: needs review labels Sep 12, 2022

tscrim added s: needs work and removed s: positive review labels Sep 12, 2022

DennisJahn added s: needs review and removed s: needs work labels Sep 13, 2022

tscrim added s: positive review and removed s: needs review labels Sep 13, 2022

mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022

vbraun removed the s: positive review label Sep 20, 2022

vbraun closed this as completed in 2a41c6e Sep 20, 2022

This was referenced Sep 14, 2022

Changing brick polytopes of subword complexes to brick polyhedra #32681

Open

Bruhat cones for Weyl groups (see #32669) #32710

Closed

Adding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py #32669

Adding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py #32669

Comments

DennisJahn commented Oct 11, 2021

DennisJahn commented Oct 12, 2021

sagetrac-git mannequin commented Oct 13, 2021

sagetrac-git mannequin commented Oct 13, 2021

fchapoton commented Oct 13, 2021

DennisJahn commented Oct 14, 2021

sagetrac-git mannequin commented Oct 14, 2021

sagetrac-git mannequin commented Oct 14, 2021

sagetrac-git mannequin commented Oct 14, 2021

sagetrac-git mannequin commented Oct 14, 2021

sagetrac-git mannequin commented Oct 15, 2021

sagetrac-git mannequin commented Oct 15, 2021

fchapoton commented Oct 15, 2021

DennisJahn commented Oct 15, 2021

sagetrac-git mannequin commented Oct 25, 2021

sagetrac-git mannequin commented Oct 25, 2021

sagetrac-git mannequin commented Oct 25, 2021

sagetrac-git mannequin commented Oct 25, 2021

kliem commented Nov 22, 2021

sagetrac-git mannequin commented Nov 23, 2021

sagetrac-git mannequin commented Nov 23, 2021

sagetrac-git mannequin commented Nov 23, 2021

sagetrac-git mannequin commented Nov 23, 2021

sagetrac-git mannequin commented Nov 24, 2021

sagetrac-git mannequin commented Nov 24, 2021

DennisJahn commented Apr 14, 2022

fchapoton commented Apr 14, 2022

DennisJahn commented Apr 14, 2022

tscrim commented Apr 16, 2022

DennisJahn commented Apr 19, 2022

stumpc5 commented Sep 12, 2022

fchapoton commented Sep 12, 2022

fchapoton commented Sep 12, 2022

stumpc5 commented Sep 12, 2022

fchapoton commented Sep 12, 2022

tscrim commented Sep 12, 2022

sagetrac-git mannequin commented Sep 13, 2022

sagetrac-git mannequin commented Sep 13, 2022

tscrim commented Sep 13, 2022

tscrim commented Sep 13, 2022

vbraun commented Sep 20, 2022