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: gh-DennisJahn Owned by:
Priority: major Milestone: sage-9.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:

Status badges

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 gh-DennisJahn

Summary: Adding upper and lower Bruhat cones of M. Dyer to Coxeter groupsAdding upper and lower Bruhat cones of M. Dyer to sage/combinat/root_system/reflection_group_real.py

comment:2 Changed 14 months ago by gh-DennisJahn

Branch: u/gh-DennisJahn/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 git

Commit: 8774d2dc763fccfa8a4124d980001b079d117ca7

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

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

comment:4 Changed 14 months ago by gh-DennisJahn

Status: newneeds_review

comment:5 Changed 14 months ago by Frédéric Chapoton

would examples with WeylGroup? or CoxeterGroup? work ?

comment:6 Changed 14 months ago by gh-DennisJahn

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 git

Commit: 8774d2dc763fccfa8a4124d980001b079d117ca7da7e6b6287d4867de201ee282553e135ea1a7300

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

da7e6b6Added zero vertex to the cone

comment:8 Changed 14 months ago by git

Commit: da7e6b6287d4867de201ee282553e135ea1a7300cce2fda76ee36bd3b358bd2cc5c615acd3ef18cd

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

cce2fdadoc syntax

comment:9 Changed 14 months ago by git

Commit: cce2fda76ee36bd3b358bd2cc5c615acd3ef18cdb9692a691663cc418e59d6e8b04c3c88c230a515

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

b9692a6Changed 'Returns' to 'Return' in the doc

comment:10 Changed 14 months ago by Frédéric Chapoton

in Weyl Groups, there is

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

comment:11 Changed 14 months ago by gh-DennisJahn

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 git

Commit: b9692a691663cc418e59d6e8b04c3c88c230a515898ddd7165e2e7c4edeb690beaf05e8322ff9c3a

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

898ddd7style changes as mentioned by gh-kliem in 32681

comment:13 Changed 14 months ago by git

Commit: 898ddd7165e2e7c4edeb690beaf05e8322ff9c3af7a19db8d6ce5c5095454fee0823e0ff693cb99d

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

f7a19dbits cdd not ccd

comment:14 Changed 13 months ago by gh-kliem

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.

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.

comment:15 Changed 13 months ago by git

Commit: f7a19db8d6ce5c5095454fee0823e0ff693cb99d37ff44a2642c40c90f060d8f8bc4784fe0e9231c

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

37ff44aadded 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 git

Commit: 37ff44a2642c40c90f060d8f8bc4784fe0e9231cc816e5543d2e6570a37554d6cca27e380e72d524

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

c816e55syntax for doc highlighting

comment:17 Changed 13 months ago by git

Commit: c816e5543d2e6570a37554d6cca27e380e72d52490393b23d1f05be6cdfe6f0d23329c503032dea7

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

90393b2better description for bruhat cone, wrapped and empty lines, corrected doctests

comment:18 Changed 13 months ago by git

Commit: 90393b23d1f05be6cdfe6f0d23329c503032dea7185407eaee2571f27522c365de29db698ef61237

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

185407eadded references

comment:19 in reply to:  14 Changed 13 months ago by gh-DennisJahn

Replying to gh-kliem:

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 to CoxeterGroups 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 Changed 12 months ago by Frédéric Chapoton

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 git

Commit: 185407eaee2571f27522c365de29db698ef61237b950d056726bf23332fa9abcbaa2b4c5675be931

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

8bfe4dachanged Tests to TESTS
1eeaa9duse the arxiv role
88e4743removed the superfluous else
b950d05added a doctest for the raise

comment:22 in reply to:  20 Changed 12 months ago by gh-DennisJahn

Replying to chapoton:

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

Thanks! I changed/added the parts according to your comment. The local doctests for reflection_group_real.py including gap3 passed.

comment:23 Changed 12 months ago by Frédéric Chapoton

the new tests for raise are not indented correctly

comment:24 Changed 12 months ago by git

Commit: b950d056726bf23332fa9abcbaa2b4c5675be9314486d0239a842810ea5f2c502e5085f79d57a116

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

4486d02corrected the indentation

comment:25 in reply to:  23 Changed 12 months ago by gh-DennisJahn

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 git

Commit: 4486d0239a842810ea5f2c502e5085f79d57a116abfb07b64fd1e16a11c1708ff5a5b93893ba30e2

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

abfb07bmoved around some spaces

comment:27 Changed 12 months ago by Frédéric Chapoton

typo in *cooresponding* (at least twice)

comment:28 Changed 12 months ago by git

Commit: abfb07b64fd1e16a11c1708ff5a5b93893ba30e28fd159d9db6c0048174210a840799350ce729438

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

8fd159dtypo correction

comment:29 Changed 12 months ago by git

Commit: 8fd159d9db6c0048174210a840799350ce7294381e3d994aa6567094255467d3eb8157feb6c69ebf

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

1e3d994extra colon for indented block

comment:30 Changed 12 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

comment:31 Changed 9 months ago by Frédéric Chapoton

Status: needs_reviewneeds_work

red branch, so needs a rebase => needs_work status

comment:32 Changed 9 months ago by git

Commit: 1e3d994aa6567094255467d3eb8157feb6c69ebf9181ed30bd3bb1301f072e1e78ef9b70f9a9cd1b

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

9f307f4added references
43846a6changed Tests to TESTS
82612d6use the arxiv role
37400d8removed the superfluous else
728a1a3added a doctest for the raise
5f1f644corrected the indentation
17e6fcamoved around some spaces
04157aetypo correction
40efbb0extra colon for indented block
9181ed3Merge branch 'u/gh-DennisJahn/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 git

Commit: 9181ed30bd3bb1301f072e1e78ef9b70f9a9cd1b1fe0f7240c8719e67a5c8fa8cbe393052aa1066e

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

deda01csyntax for doc highlighting
4c1d6c9better description for bruhat cone, wrapped and empty lines, corrected doctests
5cb51a0added references
3db92aachanged Tests to TESTS
3805d4euse the arxiv role
6966092corrected the indentation
87e8d8fmoved around some spaces
13d749dtypo correction
f190607extra colon for indented block
1fe0f72Merge branch 'u/gh-DennisJahn/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 gh-DennisJahn

Status: needs_workneeds_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 Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:36 Changed 8 months ago by Frédéric Chapoton

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 copy-paste 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 Frédéric Chapoton

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 Travis Scrimshaw

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 git

Commit: 1fe0f7240c8719e67a5c8fa8cbe393052aa1066e9ed53c18f8356582f2fa8927d5ed7f8cb5d7abf9

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

8a60078added 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
0ef52beadded the method bruhat_cone including references
9ed53c1Merge branch 'u/gh-DennisJahn/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 in reply to:  36 ; Changed 8 months ago by gh-DennisJahn

Replying to chapoton:

Your branch is a little mess, with too many commits. I would suggest to start a new branch afresh. Please just copy-paste 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 in reply to:  40 ; Changed 8 months ago by Frédéric Chapoton

Replying to gh-DennisJahn:

Replying to chapoton:

Your branch is a little mess, with too many commits. I would suggest to start a new branch afresh. Please just copy-paste 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 in reply to:  41 ; Changed 8 months ago by gh-DennisJahn

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 non-crystallographic coxeter groups and further use the implementation for subword complexes in #32681.

Last edited 8 months ago by gh-DennisJahn (previous) (diff)

comment:43 Changed 8 months ago by git

Commit: 9ed53c18f8356582f2fa8927d5ed7f8cb5d7abf957fd20c3f14b9abf71b16039048855eef4b3f95f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

comment:44 Changed 8 months ago by git

Commit: 57fd20c3f14b9abf71b16039048855eef4b3f95f84b5726017c226182cf917441afc347f22dc1b94

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

d8ccc5aadded 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
84b5726added the method bruhat_cone including references

comment:45 in reply to:  42 Changed 8 months ago by gh-DennisJahn

Replying to gh-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.

comment:46 Changed 8 months ago by Frédéric Chapoton

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 in reply to:  46 Changed 8 months ago by gh-DennisJahn

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

comment:48 Changed 8 months ago by Travis Scrimshaw

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 @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 in reply to:  48 Changed 8 months ago by gh-DennisJahn

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 Christian Stump

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 Frédéric Chapoton

Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_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 Christian Stump

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:53 Changed 3 months ago by Frédéric Chapoton

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

comment:54 Changed 3 months ago by Travis Scrimshaw

Status: positive_reviewneeds_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 git

Commit: 84b5726017c226182cf917441afc347f22dc1b94648e6340df54ecdf456a4019a84dae2853661660

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

70ba7b5added Dy1994 and JS2021 to master reference file
648e634moved references and deleted empty lines

comment:56 Changed 3 months ago by gh-DennisJahn

Status: needs_workneeds_review

comment:57 Changed 3 months ago by Travis Scrimshaw

Reviewers: Frédéric ChapotonFrédéric Chapoton, Travis Scrimshaw
Status: needs_reviewpositive_review

Thank you.

comment:58 Changed 3 months ago by Matthias Köppe

Milestone: sage-9.7sage-9.8

comment:59 Changed 3 months ago by Volker Braun

Branch: u/gh-DennisJahn/adding_upper_and_lower_bruhat_cones_of_m__dyer_to_sage_combinat_root_system_reflection_group_real_py648e6340df54ecdf456a4019a84dae2853661660
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.