sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

Changing brick polytopes of subword complexes to brick polyhedra #32681

Open DennisJahn opened 3 years ago

DennisJahn commented 3 years ago

generalize the method brick_polytope() of sage.combinat.subword_complex.py to brick_polyhedron() as done in

https://arxiv.org/abs/2103.03715

Depends on #32669

CC: @stumpc5 @jplab @kliem

Component: combinatorics

Keywords: subword complex, brick vector, brick polyhedron, brick polytope

Author: Dennis Jahn

Branch/Commit: u/gh-DennisJahn/changing_brick_polytopes_of_subword_complexes_to_brick_polyhedra @ b3abcc9

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

436a8bafixing gap3 doctests
stumpc5 commented 2 years ago
comment:36

The code looks good to me! @tscrim, @fchapoton: I am sure you have some comments, at least concerning the formatting and/or the doctests. Could you maybe have a quick look? -- Thanks!

fchapoton commented 2 years ago
comment:37

doc does not build

 /home/sagemath/sage/src/sage/combinat/subword_complex.py:docstring of sage.combinat.subword_complex.SubwordComplex.brick_polyhedron:50: WARNING: citation not found: JS2021
fchapoton commented 2 years ago
comment:38

the new deprecation must be doctested, for exemple in the module documentation near the top of the file

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 436a8ba to b3abcc9

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

b3abcc9one more function, new doctest for new deprecation
DennisJahn commented 2 years ago
comment:40

Replying to Frédéric Chapoton:

doc does not build

 /home/sagemath/sage/src/sage/combinat/subword_complex.py:docstring of sage.combinat.subword_complex.SubwordComplex.brick_polyhedron:50: WARNING: citation not found: JS2021

The reference [JS2021] was added in #32669 I added the doctest for the new deprecation

fchapoton commented 1 year ago
comment:41

I disagree with the replacement of non-gap3-optional doctests with gap3 optional doctests.

DennisJahn commented 1 year ago
comment:42

Replying to Frédéric Chapoton:

I disagree with the replacement of non-gap3-optional doctests with gap3 optional doctests.

The replacement of brick_polytope() by brick_polyhedron() requires the computation of the associated Bruhat cone which is implemented for finite real reflection groups and needs gap3.

tscrim commented 1 year ago
comment:43

Sorry it took me a while to get to this.

It shouldn't be hard to implement the bruhat_cone for Weyl groups. However, you still are removing functionality. This needs to be justified, and, if deemed to be reasonable, deprecated. At least in the short term, you can just have a isinstance(G, RealReflectionGroup): to do the new implementation while keeping the old since they are both fairly short.

Also, I think it would be easier to replace the string sign with a boolean positive[_roots].

stumpc5 commented 1 year ago
comment:44

I think using isinstance(G, RealReflectionGroup) is a reasonable workaround.