Skip to content
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

Merged
merged 14 commits into from
Jul 2, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Jun 6, 2024

Adds a constructor lie_algebra(::Field, ::RootSystem), and redirects the constructors given a Dynkin type to this instead of transforming a GAP.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 like HighestWeightModule and DominantCharacter (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 to is_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.

@lgoettgens lgoettgens added topic: LieTheory experimental Only changes experimental parts of the code WIP NOT ready for merging labels Jun 6, 2024
@lgoettgens lgoettgens force-pushed the lg/Lie-construction branch from 72a70b9 to 06167c4 Compare June 21, 2024 14:23
@lgoettgens lgoettgens removed the WIP NOT ready for merging label Jun 21, 2024
@lgoettgens lgoettgens marked this pull request as ready for review June 21, 2024 16:45
Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 99.26471% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.97%. Comparing base (1dcb279) to head (e7e6db9).

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     
Files Coverage Δ
experimental/LieAlgebras/src/iso_oscar_gap.jl 100.00% <100.00%> (ø)
...mental/LieAlgebras/test/AbstractLieAlgebra-test.jl 100.00% <100.00%> (ø)
...rimental/LieAlgebras/test/LieAlgebraModule-test.jl 100.00% <ø> (ø)
experimental/LieAlgebras/src/AbstractLieAlgebra.jl 87.17% <98.92%> (+13.78%) ⬆️

... and 1 file with indirect coverage changes

@lgoettgens
Copy link
Member Author

This needs some minor adaptions once #3894 is merged.

@lgoettgens lgoettgens force-pushed the lg/Lie-construction branch from 4388dc2 to fe5cc03 Compare July 2, 2024 13:33
@lgoettgens
Copy link
Member Author

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.

@lgoettgens lgoettgens merged commit 170478d into oscar-system:master Jul 2, 2024
27 of 29 checks passed
@lgoettgens lgoettgens deleted the lg/Lie-construction branch July 2, 2024 17:42
@benlorenz
Copy link
Member

This PR seems to have caused some memory issues on the most recent test runs on master.
Both 1.11-nightly+short and nightly+short are quite consistently failing now, it looks like they are running out of memory:

Starting tests for /home/runner/work/Oscar.jl/Oscar.jl/experimental/LieAlgebras/test/LieAlgebraModule-test.jl
...
GC: pause 2970.72ms. collected 5382.313034MB. incr 
Heap stats: bytes_mapped 8194.00 MB, bytes_resident 8194.00 MB,
heap_size 8710.34 MB, heap_target 11852.00 MB, Fragmentation 0.290

GC: pause 11435.75ms. collected 7799.750633MB. incr 
Heap stats: bytes_mapped 11266.75 MB, bytes_resident 11243.50 MB,
heap_size 11746.20 MB, heap_target 15275.70 MB, Fragmentation 0.251

[1822] signal 15: Terminated
in expression starting at /home/runner/work/_actions/julia-actions/julia-runtest/latest/test_harness.jl:7
Error: The operation was canceled.

From here: https://github.com/oscar-system/Oscar.jl/actions/runs/9793439785/job/27041381638#step:9:2000
Same error for nightly: https://github.com/oscar-system/Oscar.jl/actions/runs/9782062414/job/27027942818

The runners have 14GB RAM if I remember correctly.

@lgoettgens
Copy link
Member Author

Ouch, but interesting that this regression is not in the newly introduced tests in AbstractLieAlgebra-test.jl but in LieAlgebraModule-test.jl.
I will look into it, maybe some of my spontaneous ideas work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code topic: LieTheory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants