-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Construct Lie algebras from root system #3831
Construct Lie algebras from root system #3831
Conversation
72a70b9
to
06167c4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3831 +/- ##
==========================================
+ Coverage 83.91% 83.97% +0.05%
==========================================
Files 589 589
Lines 81054 81182 +128
==========================================
+ Hits 68020 68174 +154
+ Misses 13034 13008 -26
|
This needs some minor adaptions once #3894 is merged. |
so it doesn't use a different one (e.g. with changed order of simple roots)
4388dc2
to
fe5cc03
Compare
Rebased on top of #3894. Locally running the in-depth tests for the new feature still succeeds, so I hope that I broke nothing else. |
This PR seems to have caused some memory issues on the most recent test runs on master.
From here: https://github.com/oscar-system/Oscar.jl/actions/runs/9793439785/job/27041381638#step:9:2000 The runners have 14GB RAM if I remember correctly. |
Ouch, but interesting that this regression is not in the newly introduced tests in |
Adds a constructor
lie_algebra(::Field, ::RootSystem)
, and redirects the constructors given a Dynkin type to this instead of transforming aGAP.Globals.SimpleLieAlgebra
.(@HereAround @emikelsons this allows the construction of F4)
To the reviewers: Please don't try to understand the algorithm nor the concrete implementation, I've already spent way to much time doing that myself. And there are pretty comprehensive tests that check that the resulting Lie algebras satisfy the Jacobi identity, and that GAP can detect the Dynkin type again.
Note to GAP: The
iso_oscar_gap
method had to be adapted, because GAP by default decided to take a different Chevalley basis and in particular a different order of simple roots, which made functions likeHighestWeightModule
andDominantCharacter
(that take a weight in terms of fundamental weights) behave unexpectedly. As a workaround, we now construct and pass a GAP root system.I am aware of some optimizations in
_struct_consts
and_N_matrix
, in particular w.r.t. the number of calls tois_root_with_index
etc. That's something I plan to optimize in the future, but I would be happy to have this already merged prior to that. If you see any other obvious performance improvements, please let me know.