Add Coxeter Groups and Root Systems #2689

felix-roehrich · 2023-08-17T09:28:05Z

I am currently working on implementing https://github.com/ulthiel/CoxeterGroups.jl (see #2217). To have compatibility with root systems and since many efficient algorithms for Weyl groups rely on geometric properties, I have also written a first draft for root systems in OSCAR.
Currently this is still under heavy development, especially distributing logic between root systems and Weyl groups.
At the moment Cartan matrices are implemented as ZZMatrix, but it might make sense to implement it as a sparse matrix instead.
Documentation and tests are also missing.

For simplicity everything lives in experimental/LieAlgebras but everything from Coxeter groups that does not depend on root systems should probably moved to the Groups folder at some point.

simonbrandhorst · 2023-08-21T19:31:08Z

Note that we have root lattices in the sense of quadratic forms coming from Hecke:

julia> root_lattice(:A,4)
Integer lattice of rank 4 and degree 4
with gram matrix
[ 2   -1    0    0]
[-1    2   -1    0]
[ 0   -1    2   -1]
[ 0    0   -1    2]

Perhaps they can be of use to you.
I did not have root systems in mind when I implemented them though. But for some application I needed a highest root.

julia> highest_root(:A, 4)
[1   1   1   1]

felix-roehrich · 2023-08-22T17:57:28Z

Thanks, I'll have a look.

fingolfin · 2023-08-24T09:45:10Z

Thank you for this, I'll leave some comments later. There are many other people which are working on things in algebraic Lie theory which may have an interest in this, e.g. @lgoettgens and @voggesbe. We don't need to have this perfectly in sync but it would be good if eventually we can share types for things like root systems between Lie algebras, Coxeter/Weyl groups and other applications, so it's good to stay in touch about these things

fingolfin · 2023-08-24T09:46:52Z

One further comment: on the long run, I want to work with infinite Coxeter groups, too, and also work on code for Kac-Moody groups and algebras. For these (but also already for Lie algebras and algebraic groups) it may be useful to also have a notion of "root datum" (like Magma also has, see https://magma.maths.usyd.edu.au/magma/handbook/text/1143). I am merely saying this to set a vision for the future; this is not a request for you to change anything here right now.

lgoettgens · 2023-08-24T10:09:39Z

Thank you for this, I'll leave some comments later. There are many other people which are working on things in algebraic Lie theory which may have an interest in this, e.g. @lgoettgens and @voggesbe. We don't need to have this perfectly in sync but it would be good if eventually we can share types for things like root systems between Lie algebras, Coxeter/Weyl groups and other applications, so it's good to stay in touch about these things

@felix-roehrich has his office two doors down from mine. And we talk regularly about this, and how it could connect to the other Lie algebra stuff in Oscar.

experimental/LieAlgebras/src/AbstractRootSystem.jl

fingolfin · 2023-08-24T09:49:29Z

experimental/LieAlgebras/src/CartanMatrix.jl

+    end
+    mat[rk - 1, rk] = -2
+  elseif fam == :E
+    @req rk in 6:8 "type En requires rank to be one of 6, 7, 8"


Just as a side remark (not a change request): Actually there is also $E_9$, $E_{10}$ - the resulting Weyl/Coxeter groups, Lie algebras just are not finite dimensional anymore. I have particular interest in these, so ultimately I'd want this to work more generally.

If you have a reference to common generalized Cartan matrices, I will implement them. For now I have implemented the classical case, but when offering generalized Cartan matrices, there should be some rules on what is offered by the helper.

The standard reference for Cartan matrices (generalized) should be Kac's book Infinite Dimensional Lie Algebras I would say.

experimental/LieAlgebras/src/CoxeterGroup.jl

experimental/LieAlgebras/src/RootSystem.jl

experimental/LieAlgebras/src/WeylGroup.jl

fingolfin · 2023-08-24T09:53:07Z

experimental/LieAlgebras/src/WeylGroup.jl

+#
+###############################################################################
+
+struct WeylGroup <: CoxeterGroup


you probably will want this to be attribute storing on the long run...

Suggested change

struct WeylGroup <: CoxeterGroup

@attributes mutable struct WeylGroup <: CoxeterGroup

I think it would be useful to have a comment (or better: docstring) that briefly explains in how far you consider "Weyl group" as a special case of Coxeter group (i.e., what makes it different from "other" kinds of of Coxeter groups)

WeylGroup is the implemtation of CoxGrpMin in https://github.com/ulthiel/CoxeterGroups.jl. Since this PR is quite large already, in a follow up PR I want to add CoxGrpRec renamed to CoxeterGroupGeneric (?) and offer a generic Coxeter group constructor from a Coxeter matrix. In my opinion there should then be references for specials cases like Weyl Groups (with specialized algorithms). In ulthiel/CoxeterGroups.jl#1 CoxeterGroups{T} was suggested. I am open for suggestions also from @ulthiel.
Also note that an abstract type is necessary to break cyclic dependency between root systems and Weyl groups.

(Maybe I also misunderstood your comment.)

Whenever I read "Weyl group" I think "finite Coxeter group". Maybe I am too much used to the algebraic groups setting. Kac uses Weyl group more generally as well. I don't know what's best. But note that CoxGrpMin refers to an algorithm (namely using minimal roots). I somehow like(d) to make this clear. But again, I don't know what's best.

2 days later I really think there should be separate types for the separate algorithms for computing in (some classes of) Coxeter groups and the types should also be called like that, as originally in CoxeterGroups.jl. So, I would prefer CoxGrpMin to WeylGroup. Also, there is CoxGrpSym in CoxeterGroups.jl which is extremely useful for type-A computations. This should go in here as well. There may be CoxGrpPerm for a permutation group algorithm etc. The generic function coxeter_group(...) should select the type that's most appropriate for the given type. @fingolfin, what do you think?

I will look at this next week.

Could this be looked at before merging?

To me Weyl group applies to infinite groups as well, in Kac Moody theory at least we consistently use that.

Having different types for different algorithms makes some sense, but there are also many drawbacks. But I don't think it's necessary to hold up this PR about it, as this is purely for the experimental branch.

If we want to make progress on this, I don't think discussions on PRs are the way forward. It just doesn't work that well. We need to sit together in a room with enough time (possible organize a little workshop) and hash it out... but in the end, actual code trumps all. If something is implemented and works, great; if it doesn't work, we can iterate on it.

I.e., "use case drive design" -- which is why I said in a PR comment elsewhere that I don't want to hold up this PR right now as I just don't have time to work on it or one code using it, and if it works for you guys right now, great. I already know I will need changes for my use cases (e.g. infinite root systems should not store explicit vectors of positive roots; and I need a RootDatum type; etc.) but all of that can (and IMHO: should) be added later.

fingolfin · 2023-08-24T10:11:56Z

@felix-roehrich @lgoettgens are you also in contact with @ulthiel about this?

felix-roehrich · 2023-08-24T10:24:27Z

I am thankful for your comments but please wait until I mark the PR as ready for review. Most of the comments you just made are already outdated from my recent work and this will just create more work for both of us. I will mark your recent comments as resolved once I push my changes.

felix-roehrich · 2023-09-26T12:49:51Z

Tests and sanity checks still missing. I will add those if no more (big) changes are requested/necessary.

lgoettgens · 2023-09-26T15:28:35Z

I'll hope to find some time the next days to review this. And it would be great if @fingolfin could have a look as well.

codecov · 2023-09-27T09:07:19Z

Codecov Report

Merging #2689 (daf49e4) into master (d5e7b73) will decrease coverage by 0.31%.
The diff coverage is 52.00%.

@@            Coverage Diff             @@
##           master    #2689      +/-   ##
==========================================
- Coverage   80.38%   80.08%   -0.31%     
==========================================
  Files         469      471       +2     
  Lines       66383    66856     +473     
==========================================
+ Hits        53363    53541     +178     
- Misses      13020    13315     +295

Files	Coverage Δ
experimental/GModule/Cohomology.jl	`67.12% <ø> (-0.72%)`	⬇️
experimental/JuLie/src/JuLie.jl	`100.00% <ø> (ø)`
experimental/LieAlgebras/src/LieAlgebras.jl	`100.00% <ø> (ø)`
...mental/LieAlgebras/test/AbstractLieAlgebra-test.jl	`100.00% <ø> (ø)`
...rimental/LieAlgebras/test/LieAlgebraModule-test.jl	`90.90% <ø> (ø)`
...xperimental/LieAlgebras/test/iso_oscar_gap-test.jl	`100.00% <ø> (ø)`
experimental/LieAlgebras/src/root_systems.jl	`0.00% <0.00%> (-33.02%)`	⬇️
experimental/LieAlgebras/src/AbstractLieAlgebra.jl	`70.53% <50.00%> (-10.42%)`	⬇️
experimental/LieAlgebras/src/CartanMatrix.jl	`46.34% <46.34%> (ø)`
experimental/LieAlgebras/src/WeylGroup.jl	`67.92% <67.92%> (ø)`
... and 1 more

... and 6 files with indirect coverage changes

ulthiel · 2023-09-27T18:42:30Z

I'll test it. But before I forget: please make sure that the following people are mentioned as contributors (as well): Cameron Braunstein (@CameronBraunstein), Joel Gibson (University of Sydney, @joelgibson), Tom Schmit (@schto223). As with JuLie, if this gets merged then I'll archive CoxeterGroups.jl.

On the type design/hierarchy the experts should comment (@fingolfin). I still don't know what's the way to go. (It would be good to have Coxeter groups finally, though, in any way.)

lgoettgens · 2023-10-01T09:46:40Z

I'll hope to find some time the next days to review this. And it would be great if @fingolfin could have a look as well.

I unfortunately didn't get to this yet. And as I am on holidays until the 8th, I'll only manage to review this the week after.

fingolfin · 2023-10-11T22:41:21Z

Both PR #2572 and PR #2689 add root systems. How shall we resolve this? @felix-roehrich @voggesbe @lgoettgens ?

lgoettgens · 2023-10-12T05:49:45Z

Both PR #2572 and PR #2689 add root systems. How shall we resolve this? @felix-roehrich @voggesbe @lgoettgens ?

According to @felix-roehrich there should be no conflict in merging both after review. Afterwards, we can try to unify these. My dream would be to make the GAP-backed root systems a concrete subtype of the abstract root system type of this PR.

experimental/LieAlgebras/src/AbstractRootSystem.jl

fingolfin · 2023-10-16T14:15:55Z

My dream would be to make the GAP-backed root systems a concrete subtype of the abstract root system type of this PR.

I don't quite share that -- it sounds as if we would have multiple root system implementations that overlap in functionality but are still different. That's something I'd rather avoid. But perhaps I misunderstand?

lgoettgens · 2023-10-16T14:26:40Z

My dream would be to make the GAP-backed root systems a concrete subtype of the abstract root system type of this PR.

I don't quite share that -- it sounds as if we would have multiple root system implementations that overlap in functionality but are still different. That's something I'd rather avoid. But perhaps I misunderstand?

Let's get #2572 merged first. Afterwards, I can work with @felix-roehrich on making them compatible / merging them /... let's see how it evolves.
I find it quite hard to reason about it currently with everything living in two different PRs. With one in master and the other one in a PR, this will be a lot simpler.

And after all, this is still in experimental.

felix-roehrich · 2023-10-16T14:58:36Z

Let's get #2572 merged first. Afterwards, I can work with @felix-roehrich on making them compatible / merging them /... let's see how it evolves.

If I didn't miss anything #2572 only uses GAP only to get basic root system information (like number of roots, Cartan matrix). So this can easily be replaced with the implementation in this PR.

experimental/LieAlgebras/test/WeylGroup-test.jl

experimental/LieAlgebras/test/RootSystem-test.jl

experimental/LieAlgebras/test/runtests.jl

@felix-roehrich

@felix-roehrich and I talked about this yesterday and pushed some changes

lgoettgens · 2023-10-24T10:20:46Z

This is now only missing a fix for the AbstractLieAlgebra constructor. I will work on this this afternoon.

lgoettgens

This is now good to go from my POV. Further functionalities, tests, and docs will be added by @felix-roehrich in a future PR.

Can somebody with merge rights (maybe @thofma) enable auto-squash-merge on this?

lgoettgens · 2023-10-25T10:37:29Z

@fingolfin please let us know if you have any urgent changes to be made in this PR. If not, please merge this and you can write comments afterward that get addressed in a follow-up.

ulthiel · 2023-10-25T16:32:11Z

@fingolfin please let us know if you have any urgent changes to be made in this PR. If not, please merge this and you can write comments afterward that get addressed in a follow-up.

I made several comments/questions above on types and naming. There was no reply.

lgoettgens · 2023-10-25T17:01:56Z

@fingolfin please let us know if you have any urgent changes to be made in this PR. If not, please merge this and you can write comments afterward that get addressed in a follow-up.

I made several comments/questions above on types and naming. There was no reply.

Ah, sorry, I somehow missed them while looking through this PR. I'll give an answer here.

We decided to (for the moment at least; this can of course be renamed after a discussion) keep the name WeylGroup for the following reasons:

OSCAR type names should convey mathematical meaning. Your suggestion CoxGrpMin is a weird combination of abbreviations (that are not to be used due to the OSCAR style guide, unless the exemptions specified there). Every mathematician not knowing about the specific internals of the implementation will be confused of the name. And if I understand the OSCAR philosophy right, we don't want users to think about such internals.
In the current only use case (root systems of Lie algebras), the terminology of a Weyl group of that root system is most commonly used.
Some explanation about the concrete algorithm used will be added to the docs. The same holds for the use of the word "Weyl group", which is used in a very general manner like Kac.

Other subtypes of CoxeterGroup will get implemented by @felix-roehrich add a later point in time.
Your suggestion to have a coxeter_group function that chooses the implementation based on the input will, with very high probability, be type instable. So I would refrain from using this in any library code. For user convenience, we can talk about it once there are multiple types to choose from.

All of this is of course not set in stone and can be changed in a future PR.
But since this PR is open for already more than 2 months, and I would like to use parts of it in #2936, I would like to get this going.

@felix-roehrich if I forgot something, please add.

@fingolfin already approved this in a very similar state. Could you please give your opinion to this?

ulthiel · 2023-10-26T06:02:25Z

Alright, good, it seems you thought about this, that's all I wanted. I want to protest though against calling my idea a "weird combination" :-D (joking)!

fingolfin

Fine by me.

I left a few comments on type stability, outdated docstrings etc.

As I understood @lgoettgens on Friday he'd prefer to apply the corresponding fixes post-merge. Fine by me, too.

fingolfin · 2023-10-28T09:49:24Z

experimental/LieAlgebras/src/AbstractLieAlgebra.jl

+@doc raw"""
+    chevalley_basis(L::AbstractLieAlgebra{C}) -> NTuple{3,Vector{AbstractLieAlgebraElem{C}}}
+
+Return the Chevalley basis of the Lie algebra `L` in three vectors, stating first the positive root vectors, 


You say "the" Chevalley basis, "the" Cartan subalgebra. These are not in general unique things, so what is meant? IMHO this should either be changed to "a" in both cases, or somewhere should be an explanation in how far something "unique" is returned that warrants the use "the"

fingolfin · 2023-10-28T09:50:13Z

experimental/LieAlgebras/src/AbstractLieAlgebra.jl

+to the order of the roots in the root system.
+"""
+function chevalley_basis(L::AbstractLieAlgebra)
+  @req has_root_system(L) "No root system known."


Wouldn't root_system(L) below already throw a similar error? Removing this check here then would have the advantage that this method would start working once someone implements runtime computation of root systems...

fingolfin · 2023-10-28T09:52:01Z

experimental/LieAlgebras/src/AbstractLieAlgebra.jl

    lie_algebra(R::Field, dynkin::Tuple{Char,Int}; cached::Bool) -> AbstractLieAlgebra{elem_type(R)}

 Construct the simple Lie algebra over the ring `R` with Dynkin type given by `dynkin`.


Docstring doesn't match changed signature

Suggested change

lie_algebra(R::Field, dynkin::Tuple{Char,Int}; cached::Bool) -> AbstractLieAlgebra{elem_type(R)}

Construct the simple Lie algebra over the ring `R` with Dynkin type given by `dynkin`.

lie_algebra(R::Field, S::Symbol, n::Int; cached::Bool) -> AbstractLieAlgebra{elem_type(R)}

Construct the simple Lie algebra over the ring `R` with Dynkin type given by `S` and `n`.

fingolfin · 2023-10-28T09:53:36Z

experimental/LieAlgebras/src/CartanMatrix.jl

@@ -0,0 +1,144 @@
+###############################################################################
+#


Suggested change

#

experimental/LieAlgebras/src/CartanMatrix.jl

fingolfin · 2023-10-28T10:02:30Z

experimental/LieAlgebras/src/RootSystem.jl

+  positive_roots::Any #::Vector{RootSpaceElem} (cyclic reference)
+  weyl_group::Any     #::WeylGroup (cyclic reference)


Couldn't this at least be, say,

Suggested change

positive_roots::Any #::Vector{RootSpaceElem} (cyclic reference)

weyl_group::Any #::WeylGroup (cyclic reference)

positive_roots::Vector #::Vector{RootSpaceElem} (cyclic reference)

weyl_group::CoxeterGroup #::WeylGroup (cyclic reference)

The entries could be made concrete via type parameters. If it ever matters (right now I guess it doesn't).

experimental/LieAlgebras/src/CoxeterGroup.jl

fingolfin · 2023-10-28T10:05:39Z

experimental/LieAlgebras/src/RootSystem.jl

+end
+
+function num_positive_roots(R::RootSystem)
+  return length(R.positive_roots)


This suffers from type instability, can be solved via

Suggested change

return length(R.positive_roots)

return length(positive_roots(R))

Same for most other places accessing R.positive_roots instead of positive_roots(R)

fingolfin · 2023-10-28T10:06:21Z

experimental/LieAlgebras/src/RootSystem.jl

+end
+
+function positive_root(R::RootSystem, i::Int)
+  return R.positive_roots[i]::RootSpaceElem


Better:

Suggested change

return R.positive_roots[i]::RootSpaceElem

return positive_roots(R)[i]

fingolfin · 2023-10-28T10:21:09Z

experimental/LieAlgebras/src/WeylGroup.jl

+#
+###############################################################################
+
+struct WeylGroup <: CoxeterGroup


To me Weyl group applies to infinite groups as well, in Kac Moody theory at least we consistently use that.

Having different types for different algorithms makes some sense, but there are also many drawbacks. But I don't think it's necessary to hold up this PR about it, as this is purely for the experimental branch.

If we want to make progress on this, I don't think discussions on PRs are the way forward. It just doesn't work that well. We need to sit together in a room with enough time (possible organize a little workshop) and hash it out... but in the end, actual code trumps all. If something is implemented and works, great; if it doesn't work, we can iterate on it.

I.e., "use case drive design" -- which is why I said in a PR comment elsewhere that I don't want to hold up this PR right now as I just don't have time to work on it or one code using it, and if it works for you guys right now, great. I already know I will need changes for my use cases (e.g. infinite root systems should not store explicit vectors of positive roots; and I need a RootDatum type; etc.) but all of that can (and IMHO: should) be added later.

felix-roehrich mentioned this pull request Aug 17, 2023

Add Coxeter Groups and Root Systems #2686

Closed

fingolfin closed this Aug 24, 2023

fingolfin reopened this Aug 24, 2023

fingolfin approved these changes Aug 24, 2023

View reviewed changes

felix-roehrich force-pushed the fr-coxeter branch 2 times, most recently from 13d2e6c to fbe4767 Compare September 13, 2023 20:22

felix-roehrich force-pushed the fr-coxeter branch from fbe4767 to 322eaa3 Compare September 26, 2023 12:31

felix-roehrich marked this pull request as ready for review September 26, 2023 12:52

lgoettgens requested review from fingolfin and lgoettgens September 26, 2023 15:28

fingolfin mentioned this pull request Oct 11, 2023

Coxeter groups #2217

Closed

lgoettgens mentioned this pull request Oct 12, 2023

Simple Lie algebras and root systems #2572

Merged

lgoettgens reviewed Oct 13, 2023

View reviewed changes

experimental/LieAlgebras/src/AbstractRootSystem.jl Outdated Show resolved Hide resolved

lgoettgens reviewed Oct 23, 2023

View reviewed changes

experimental/LieAlgebras/test/WeylGroup-test.jl Outdated Show resolved Hide resolved

experimental/LieAlgebras/test/RootSystem-test.jl Outdated Show resolved Hide resolved

experimental/LieAlgebras/test/runtests.jl Outdated Show resolved Hide resolved

lgoettgens force-pushed the fr-coxeter branch from 18e8e93 to cd56668 Compare October 24, 2023 07:36

felix-roehrich and others added 12 commits October 24, 2023 09:37

squash commits

b1bd083

Merge AbstractRootSystem and RootSystem

c595449

Address comments

9abff60

Merge root system types

7362bd7

Merge SimpleLieAlgebra into AbstractLieAlgebra

1c63790

Fix exports

cb260ad

Try to fix everything

82d3886

Add credits

763dc49

reorder functions

cbed9dd

make Ghislain happy

5ba0c70

add some random tests

baeb4db

format files

cd56668

Add temporary workaround to make CI green again

0352d9f

lgoettgens approved these changes Oct 24, 2023

View reviewed changes

add missing accessors

daf49e4

fingolfin approved these changes Oct 28, 2023

View reviewed changes

fingolfin merged commit e1d085c into oscar-system:master Oct 28, 2023
17 of 19 checks passed

felix-roehrich deleted the fr-coxeter branch October 28, 2023 10:41

felix-roehrich mentioned this pull request Nov 3, 2023

Add docs and tests for CartanMatrix.jl #2982

Merged

lgoettgens added the topic: LieAlgebras label Nov 20, 2023

felix-roehrich mentioned this pull request Nov 27, 2023

More functionality for Weyl groups #3051

Merged

lgoettgens mentioned this pull request Feb 20, 2024

Remove unneeded GAP package sla #3398

Merged

	struct WeylGroup <: CoxeterGroup
	@attributes mutable struct WeylGroup <: CoxeterGroup

		lie_algebra(R::Field, dynkin::Tuple{Char,Int}; cached::Bool) -> AbstractLieAlgebra{elem_type(R)}

		Construct the simple Lie algebra over the ring `R` with Dynkin type given by `dynkin`.

		@@ -0,0 +1,144 @@
		###############################################################################
		#

		positive_roots::Any #::Vector{RootSpaceElem} (cyclic reference)
		weyl_group::Any #::WeylGroup (cyclic reference)

	return length(R.positive_roots)
	return length(positive_roots(R))

	return R.positive_roots[i]::RootSpaceElem
	return positive_roots(R)[i]

Add Coxeter Groups and Root Systems #2689

Add Coxeter Groups and Root Systems #2689

Conversation

felix-roehrich commented Aug 17, 2023

simonbrandhorst commented Aug 21, 2023

felix-roehrich commented Aug 22, 2023

fingolfin commented Aug 24, 2023

fingolfin commented Aug 24, 2023

lgoettgens commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Aug 24, 2023

felix-roehrich commented Aug 24, 2023

felix-roehrich commented Sep 26, 2023

lgoettgens commented Sep 26, 2023

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

ulthiel commented Sep 27, 2023

lgoettgens commented Oct 1, 2023

fingolfin commented Oct 11, 2023

lgoettgens commented Oct 12, 2023

fingolfin commented Oct 16, 2023

lgoettgens commented Oct 16, 2023

felix-roehrich commented Oct 16, 2023

lgoettgens commented Oct 24, 2023

lgoettgens left a comment

Choose a reason for hiding this comment

lgoettgens commented Oct 25, 2023

ulthiel commented Oct 25, 2023

lgoettgens commented Oct 25, 2023 • edited Loading

ulthiel commented Oct 26, 2023

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2023 •

edited

Loading

lgoettgens commented Oct 25, 2023 •

edited

Loading