#15375: Extended Affine Weyl Groups SD40
-------------------------------------+-------------------------------------
Reporter: bump | Owner: bump
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-6.8
Component: combinatorics | Resolution:
Keywords: days54, coxeter, | Merged in:
days64, days65 | Reviewers: Dan Bump, Anne
Authors: Daniel Bump, Dan | Schilling
Orr, Anne Schilling, Mark | Work issues:
Shimozono, Nicolas Thiery. | Commit:
Report Upstream: N/A | 2b78b7bdc2db6835dadfad978e5f6992f0546649
Branch: | Stopgaps:
public/combinat/extended_affine_weyl_groups-15375|
Dependencies: #10963, #14102 |
-------------------------------------+-------------------------------------
Comment (by nthiery):
Hi Mark!
I have glanced half way through the patch. It's a lot of hard work you
did; thanks! I'll try to go through the over half soon, but won't have
the time for a systematic review, so please someone!
Here are some thoughts that popped up; take them as they just are:
random food for thoughts. If something does not sound applicable, just
ignore it.
- `weyl_groups.py` line 420,453: any reason to remove the test for
s2s1s2s1?
- `RootSystem(XXX.cartan_type())` -> `XXX.cartan_type().root_system()`
- `def to_ambient_space_morphism`: you can use
`sage.misc.c3_controlled.identity`. Yeah, there is no reason why the
identity function is defined in this module; but at least if we all
use the same identity function it will be easier to refactor later
on.
Variants, if you want a morphism: `End(self).identity()` or `sage:
self.coerce_map_from(self)`,
- `..warning` -> `.. WARNING`
- In `to_weight_space`: instead of going through a vector, one can use
`.sum_of_terms([i,self.scalar(alpha[i])] for ...`
This method is generic and would better be in `WeightSpaceRealizations`.
- `to_ambient`
- `special_nodes`: any reason not to use `return
autos.orbit(self.special_node())` ?
- `Nicolas Thiery` -> `Nicolas M. Thiery` for consistency
- Doc of `ExtendedAffineWeylGroup`: use `.. MATH` for the math
formulas on their own line.
- `... the following lattices:`; idem for `E` below.
- When natural, put the `::` at the end of the sentence.
- Line folding could be more consistent
- There are still quite a few of `ifs` depending on
twisted/untwisted/GL; any chance to make this more generic?
For example, to define the special translation covector can't we
generically use something like `c[special_node] *
_special_root.associated_coroot()`? I never remember from the top of
my head whether it's `c` or one of its friends that should be used
here, but we did things like this in the MacDo code.
Note that at this point the initialization of
`ExtendedAffineWeylGroup_class` will probably break on a relabelled
type BC.
- Do we need the `special_nodes` method on the group and the
fundamental group? Or could just have it on the cartan type?
- Strip new lines just before and just after the end of the
documentation string.
- `classical_weyl` -> `classical`? or `classical_weyl_group`?
I don't remember what we did in similar situations for crystals
and the like, but please check for consistency.
- `classical_weyl_to_affine_morphism`: the name is misleading since
the result is not a morphism. Could we have a name like
`from_classical` or make this an element method `to_affine`?
Same for the related methods.
- `cardinality` is already provided by `Sets().Infinite()`
- `group_generators` this could in principle go in
`Groups().WithRealizations()`. If creating the latter just for this
seems like overkill, you can just add a comment about this.
- `...Realizations.ParentMethods.one`: replacing `PW0` by
`a_realization` the code would be generic and could go in
`Magmas.Unital.Realizations`.
- `Returns` -> `Return`
- `coset_representative, is_grassmanian, ...`: this is the same logic
as for a usual coxeter group, right? Could we avoid the duplication
by having a common super category?
- `get_the_index` -> `leading_support`?
- `FundamentalGroup...one`: `0` -> `special_node`
- `an_element`: you can probably use `.last()` (haven't tested)
- `action`: maybe `representation`? Elsewhere in Sage, `action` is
usually a method that implements the action itself.
- `root_lattice_realizations`: `Algebras` is now imported twice.
Cheers,
Nicolas
--
Ticket URL: <http://trac.sagemath.org/ticket/15375#comment:94>
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 http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
