Simple Lie algebras and root systems

[voggesbe]

I have added to files to the LieAlgebras folder in experimental that define root systems and gives the possibility to create simple Lie algebras over any ring based on a Dynkin type.

[lgoettgens]

Thanks for your contribution! I will have an in-depth look in the upcoming days.

[fingolfin]

Some quick comments

[codecov]

Codecov Report

Merging #2572 (e766b79) into master (f9d8294) will increase coverage by 36.65%.
Report is 1 commits behind head on master.
The diff coverage is 48.14%.

@@             Coverage Diff             @@
##           master    #2572       +/-   ##
===========================================
+ Coverage   43.74%   80.40%   +36.65%     
===========================================
  Files         458      467        +9     
  Lines       65036    66226     +1190     
===========================================
+ Hits        28451    53246    +24795     
+ Misses      36585    12980    -23605     

Files	Coverage Δ
experimental/LieAlgebras/src/LieAlgebra.jl	86.62% <100.00%> (+66.11%)	⬆️
experimental/LieAlgebras/src/LieAlgebras.jl	100.00% <ø> (ø)
experimental/LieAlgebras/test/LieAlgebra-test.jl	95.89% <100.00%> (+95.89%)	⬆️
src/Oscar.jl	60.52% <ø> (+1.31%)	⬆️
experimental/LieAlgebras/src/simple_lie_algebra.jl	75.47% <75.47%> (ø)
experimental/LieAlgebras/src/root_systems.jl	33.01% <33.01%> (ø)

... and 323 files with indirect coverage changes

[lgoettgens]

I have a few remarks (see below), some minor and some more critical ones (e.g. the large duplication with existing code). Please adapt the settings of your editor to honor the .editorconfig and .JuliaFormatter.toml, so that the formatting is consistent with the rest of the oscar project.
Once you adapted the formatting and addressed most of the comments, I will have another look :)
Missing documentation is not a blocker for me, that can be added in a later PR.

[lgoettgens]

It would be great if this would work without creating another GAP Lie algebra, but not a blocker.

[voggesbe]

fixed this so it now uses the structure constants via bracket. However, I now define another Lie algebra to get the structure constants, so I'm not sure if this is better in terms of defining GAP Lie algebras.

[voggesbe]

Sorry, something went wrong when I pushed the changes to Git and it reverted to the old version. I will fix it as soon as possible

[lgoettgens]

Thanks for addressing all the comments! I have a few more, but most are just minor things, typos, etc. For the formatting and some of the tests I have pushed two commits to your branch. That seemed easier for me than to explain what I mean there. Once these are resolved and the CI succeeds, this is good to go from my point of view.

A few things that are still missing, but that can surely be addressed in a future PR are:

• more tests for root systems
• add a documentation page for all of the new strings
• thinking about merging SimpleLieAlgebra and AbstractLieAlgebra with some optional attributes. There currently seems to be a lot of code duplication that maybe can be avoided this way. I need to think a bit about the consequences of that.
• Remove workarounds to deal with simple Lie algebras in Oscar without this PR (TODO for me once this is merged)

[lgoettgens]

All three versions of adjoint_matrix work for general Lie algebras, not just for SimpleLieAlgebra. Could you, therefore, please move them to LieAlgebra.jl and adapt the docstrings? In particular, describe the used basis as the one returned by basis, and not as Chevalley basis as that is not always the case then.

[voggesbe]

I have moved this to LieAlgebra.jl and adapted it a bit

[lgoettgens]

This has been moved to here, so that the conformance test is available.

[fingolfin]

@voggesbe @lgoettgens I think overall this is a nice addition, and seeing as it is in experimental, we could of course merge it as-is and improve it later; but then there should be a plan and a promise that the improvements will come. (And of course once it is merged, also others can actively help improving it instead of just commenting from the sidelines).

Of course there also some change suggestions here that could just be applied with the press of a button; dunno if @voggesbe wants that or not, or if there are also technical complications... ?

I wonder what the best path forward is then. @voggesbe what would you prefer? Perhaps it would make sense to meet (on gather.town or whatever) and talk about it and possibly "get it done"?

[voggesbe]

@voggesbe @lgoettgens I think overall this is a nice addition, and seeing as it is in experimental, we could of course merge it as-is and improve it later; but then there should be a plan and a promise that the improvements will come. (And of course once it is merged, also others can actively help improving it instead of just commenting from the sidelines).

Of course there also some change suggestions here that could just be applied with the press of a button; dunno if @voggesbe wants that or not, or if there are also technical complications... ?

I wonder what the best path forward is then. @voggesbe what would you prefer? Perhaps it would make sense to meet (on gather.town or whatever) and talk about it and possibly "get it done"?

We can definitely do that, I'm sorry about not working on it in the last few weeks but I had an operation and am still on sick leave. I'll look into it as soon as I'm better.

[fingolfin]

@voggesbe by all means, take your time! I did not mean to pressure you. I simply wanted to convey that your contribution is welcome and wanted -- despite all those "annoying" change requests by @lgoettgens and myself. Those comment are of course meant to be helpful, but I know how tiresome it can be to get a barrage of change requests when all you want is to implement what you need to solve a specific problem.

But I am happy to working on this whatever why works well for you -- if that means waiting till you recover and have time to possibly address some suggestions first, that's perfectly fine. Just don't get frustrated, and let's talk when you feel something is not going well :-)

[voggesbe]

@voggesbe what is the status of this? I would really like this to go forward to allow progress on #2689 and #2936. If I hear no objection, I will thus go ahead and apply my suggestions at the beginning of next week.

I hope I haven't missed anything, I'm sorry for not putting in so much work recently, unfortunately I had a lot of other things to take care of!
I would prefer if we can eventually allow Lie algebras to be defined over any ring, as this is one of the main requirements for the problem I'm trying to solve.

[lgoettgens]

I hope I haven't missed anything, I'm sorry for not putting in so much work recently, unfortunately I had a lot of other things to take care of!

No worries :) There were some minor syntax errors and typos left, that I just fixed. And now I am happy to be able to use it.

I would prefer if we can eventually allow Lie algebras to be defined over any ring, as this is one of the main requirements for the problem I'm trying to solve.

This was actually possible until very recently, but I removed it due to correctness doubts for some algorithms (see #2901). If somebody (maybe @ fingolfin or you) in the future wants to reenable that, they would need to check that each function available for Lie Algebras actually works over general rings and then adapt the signatures accordingly.

[lgoettgens]

This is now good to go from my pov (if CI succeeds).

Note to whoever may merge this: This PR will create conflicts with #2810. Please give me some time in-between merges to fiddle that out.

[lgoettgens]

Docs broken. Try to fix it temporarily