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

[Lie algebras] misc bugfixes, root system in Lie alg interface, dim_of_simple_module implementation in julia #4030

Merged
merged 14 commits into from
Aug 21, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Aug 20, 2024

contains:

  • misc bugfixes (missing @attr, typos)
  • some progress towards a unified root system interface for all Lie algebra subtypes
  • add a pure julia implementation of Weyl's dimension formula that is used if a root system is available (instead of converting to GAP). @benlorenz this is a first step towards Make LieAlgebraModule tests better gc-able #3913 (comment)

This contains all parts of #3980 that are not serialization related to allow for a quicker merge of the bugfixes also included there.

@lgoettgens lgoettgens added topic: LieAlgebras experimental Only changes experimental parts of the code labels Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 92.96875% with 9 lines in your changes missing coverage. Please review.

Project coverage is 84.58%. Comparing base (6caa552) to head (b08d84f).
Report is 3 commits behind head on master.

Files Patch % Lines
experimental/LieAlgebras/src/LieAlgebra.jl 14.28% 6 Missing ⚠️
experimental/LieAlgebras/src/RootSystem.jl 93.75% 2 Missing ⚠️
experimental/LieAlgebras/test/setup_tests.jl 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4030      +/-   ##
==========================================
+ Coverage   84.56%   84.58%   +0.02%     
==========================================
  Files         597      597              
  Lines       82197    82237      +40     
==========================================
+ Hits        69508    69559      +51     
+ Misses      12689    12678      -11     
Files Coverage Δ
experimental/LieAlgebras/src/AbstractLieAlgebra.jl 91.32% <100.00%> (+4.08%) ⬆️
...xperimental/LieAlgebras/src/DirectSumLieAlgebra.jl 89.04% <100.00%> (ø)
experimental/LieAlgebras/src/LieAlgebraModule.jl 90.59% <100.00%> (+0.05%) ⬆️
experimental/LieAlgebras/src/WeylGroup.jl 87.90% <100.00%> (+0.03%) ⬆️
...mental/LieAlgebras/test/AbstractLieAlgebra-test.jl 100.00% <ø> (ø)
...rimental/LieAlgebras/test/LieAlgebraModule-test.jl 100.00% <ø> (ø)
experimental/LieAlgebras/test/RootSystem-test.jl 100.00% <100.00%> (ø)
experimental/LieAlgebras/test/setup_tests.jl 95.27% <88.88%> (-0.42%) ⬇️
experimental/LieAlgebras/src/RootSystem.jl 85.14% <93.75%> (+2.34%) ⬆️
experimental/LieAlgebras/src/LieAlgebra.jl 83.13% <14.28%> (-3.04%) ⬇️

... and 2 files with indirect coverage changes

return L.root_system
end

function chevalley_basis(L::AbstractLieAlgebra)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this function was only moved, why was the doc string deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring is now attached to chevalley_basis(::LieAlgebra), as it contained nothing specific for the Lie algebra subtype

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

Return whether a root system for `L` is known.

This function should be implemented by subtypes that support root systems.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another off-topic comment, but: perhaps we should agree to whom docstrings of exported (documented) functions are addressed. I'd say it should be users. Of course sometimes there is also a point in adding some remarks for implementors. But I think those then should be marked explicitly as such, e.g. via a callout (so the !!! syntax). That said, in this case I think one could just rephrase it slightly:

Suggested change
This function should be implemented by subtypes that support root systems.
This is only implemented for Lie algebras that support root systems.

or so?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the remark. I collected the information about which functions need to be implemented by subtypes that support root systems to the general list about subtype-implemented functions to the top. Furthermore, the docstrings are adapted to reflect what users need to know. (Note that I don't have to mention subtypes with no root system support there, as in these cases has_root_system should always return false, i.e. the NotImplemented() should never trigger.). Changes in 7ea927d

@lgoettgens lgoettgens merged commit a7e4188 into oscar-system:master Aug 21, 2024
28 checks passed
@lgoettgens lgoettgens deleted the lg/Lie-algebra-misc branch August 21, 2024 11:07
HechtiDerLachs pushed a commit to HechtiDerLachs/Oscar.jl that referenced this pull request Sep 13, 2024
…of_simple_module` implementation in julia (oscar-system#4030)

* Add `!` to `set_root_system_type`

* Add `has_root_system` to Lie algebra interface

* Enhance some root system tests

* Add `root_system` to Lie algebra interface

* Add missing `@attr`

* Implement `dim_of_simple_module` in julia (in some cases)

* Revert "Skip one excessive test"

This reverts commit 0932db8 from
oscar-system#3913.

* Test some more code paths in `dim_of_simple_module`

* Move `dim_of_simple_module` to the root system

* Remove `is_integral`

* Copy fix for `id \in W`

* Cache `cartan_symmetrizer`

* Make root system interface a bit clearer

* Update experimental/LieAlgebras/src/AbstractLieAlgebra.jl
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: LieAlgebras
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants