#20402: Make subword complexes compatible with real reflection groups
-------------------------------------+-------------------------------------
Reporter: stumpc5 | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-7.2
Component: combinatorics | Resolution:
Keywords: reflection group, | Merged in:
coxeter group, subword complex, | Reviewers:
days80 | Work issues:
Authors: Christian Stump | Commit:
Report Upstream: N/A | 295d784db0ae24bed97ed7b4d3777df9dbd652c2
Branch: u/stumpc5/20402 | Stopgaps:
Dependencies: #11187 |
-------------------------------------+-------------------------------------
Comment (by tscrim):
Replying to [comment:27 stumpc5]:
> Replying to [comment:25 tscrim]:
>
> > For the speed issues, how much of this is making a difference?
>
> The complete implementation depends on {{{has_descents}}} and
multiplication of elements to be fast (and this will get another boost for
{{{ReflectionGroup}}} in #20484), when you {{{%prun}}} the code for
{{{CoxeterGroup}}} you don't see a single bottleneck but that it is too
slow in many parts (I am not complaining about this speed issue in
general, this implementation is uniform for all Coxeter systems, finite or
infinite, so that's great and the speed is what one gets there!)
That is not answering the question I asked. I asked about having one
additional level of indirection to have a specialized function for when
the input is an instance of `RealReflectionGroup`.
> > If it does, you can separate out the important functionality into a
separate method, and then subclass the general class which special cases
the methods when you use `ReflectionGroup`.
>
> This implementation of subword complexes is **not** a general
implementation that could be moved up the hierarchy, and then a few
methods could be replaced for concrete implementations. All the internal
core methods are written so that internally it only plays with numbers and
permutations of numbers, you only see roots popping out when using the
API.
I'm sure there are only some key parts of it which are written that
strongly depend on the input format. These could be abstracted to work in
general with a special subclass which has the specialized method or into
the implementations of the Coxeter group. This is a standard design
pattern for code that I've seen many times over (we even have the former
in Sage's matrix code).
> Is is indeed possible to write a toy (in terms of speed) implementation
for the category of {{{CoxeterGroups}}}, but I am not the one doing that
at the moment -- what I provide is a research level implementation for
{{{ReflectionGroup}}}.
In many ways, we are beating a dead horse as you have basically done what
I am asking for already (with the specializing being done in the different
implementations of a Coxeter group).
> > However, let me be a bit more stern, I will not set this ticket to a
positive review when you essentially remove doctest coverage because
`gap3` is not an optional package yet (although this argument weakens once
it is). Moreover, you can test all of the functionality with standard
Sage, which such tests should be localized to each method, not (only) in a
class-level test which does the "core" functionality.
>
> Hm -- you see my complaints about that above. On the other hand, I also
see your point of getting rotten untested code after a while, and I don't
really care if we pollute the documentation with more tests.
Then let's leave those tests in. That is the only thing I am asking for
here.
> > By renaming `action_on_root_indices`, you need to deprecate it.
>
> It was only there for a few months, see #11010, and this method is
almost surely not used by anyone so far. You still think it should be
deprecated ?
I am pretty sure it appears in a stable release (i.e., 7.1), so yes.
--
Ticket URL: <http://trac.sagemath.org/ticket/20402#comment:37>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
