Import code from the project basisLieHighestWeight #2115
Conversation
lkastner
requested review from
lkastner and
marabelotti
and removed request for
lkastner and
marabelotti
BenWilop
force-pushed
the
bwilop/basisLieHighestWeight
branch
from
f3bcd0b to
e811583
Compare
There was a problem hiding this comment.
Hi there, thanks for your contribution! Could you elaborate a bit about the background of this, and also your own? I.e, what are you trying to achieve here? Are you working for someone involved in the OSCAR project?
Unfortunately this code has substantial issues. In its current form, it is unsuitable for merging, not even into experimental. But if you are interested in improving it: I left a bunch of comments indicating major issues. Once those are addressed, I'd be happy to go again over the code and point out less severe issues.
| using SparseArrays | ||
|
|
||
| ZZ = Int | ||
| TVec = SparseVector{ZZ, Int} #--- Werte sind ZZ, Indizies sind Int (TVec ist Datentyp der Basisvektoren in basisLieHighestWeight) |
| end | ||
|
|
||
| return mats | ||
| end |
There was a problem hiding this comment.
Please make sure your source files end with a newline, e.g. by configured your editor to honor our .editorconfig, as described in the OSCAR Developer Documentation.
Project.toml
Outdated
| @@ -19,6 +20,7 @@ Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" | |||
| RandomExtensions = "fb686558-2515-59ef-acaa-46db3789a887" | |||
| RecipesBase = "3cdcf5f2-1ef4-517c-9805-6587b60abb01" | |||
| Singular = "bcd08a7b-43d2-5ff7-b6d4-c458787f915c" | |||
| SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf" | |||
There was a problem hiding this comment.
I am somewhat reluctant to add a depdency on SparseArrays as we have our own sparse matrix type. But I guess it depends on what you use it for, which is not clear to me.
There was a problem hiding this comment.
This changed now and the code uses only the Oscar-Type now. But there are operations that are not supported in Oscar, such as SparseMatrix*SparseMatrix and the Kronecker-Product.
There was a problem hiding this comment.
I am confused; what does "This changed now" mean? SparseArrays still is listed here (as is Distributed)
| """ | ||
| Creates the Lie-algebra as a GAP object that gets used for a lot other computations with GAP | ||
| """ |
There was a problem hiding this comment.
Docstrings must come before the function, not inside it.
| end | ||
| addAndReduce!(space[wt], vec) | ||
| end | ||
| GC.gc() |
There was a problem hiding this comment.
I don't know why, but the garbage collector of Julia sometimes doesn't delete the dereferenced objects of the function add_known_monomial and add_new_monomials. Therefore, I had memory problems without calling it manually. I did not yet find a minimal working example and can't figure out the cause. After completing the other major changes, I will test it again and try to find another solution.
| @@ -0,0 +1,110 @@ | |||
| # manages methods for different orders of monomials | |||
There was a problem hiding this comment.
OSCAR already has a system of monomial orderings, any reason you are not using that?
There was a problem hiding this comment.
I will switch to the Julia-Type, but both an older version of the code and parts of the code that create monomials through integer points of a polytope made it easier for me to keep the integers.
We opened this PR so we can work on this together and have the tests run. It will take a while to straighten out, hence it is a draft. We will also add a better description later. |
|
@lkastner In view of the PBWDeformations.jl things, do you think it would make sense to call these |
BenWilop
force-pushed
the
bwilop/basisLieHighestWeight
branch
2 times, most recently
from
aaaabe5 to
02b10b7
Compare
|
Hi everyone, thanks for all the comments! I am working as a student assistant for Ghislain Fourier in Aachen. Lars and Mara are not responsible for the project and only helped me getting started with the integration. I took over the project 1 year ago and did not change design choices that probably happened with regards to an older status of Oscar. I will try to fix all issues that come up. There are many of parts of the code that I did not clean up (such as comments) and I will do that as well. |
BenWilop
force-pushed
the
bwilop/basisLieHighestWeight
branch
from
02b10b7 to
1ec8173
Compare
BenWilop
force-pushed
the
bwilop/basisLieHighestWeight
branch
from
1ec8173 to
e581b4b
Compare
…m/Oscar.jl into bwilop/basisLieHighestWeight
…m/Oscar.jl into bwilop/basisLieHighestWeight
Project.toml
Outdated
| @@ -19,6 +20,7 @@ Random = "9a3f8284-a2c9-5f02-9a11-845980a1fd5c" | |||
| RandomExtensions = "fb686558-2515-59ef-acaa-46db3789a887" | |||
| RecipesBase = "3cdcf5f2-1ef4-517c-9805-6587b60abb01" | |||
| Singular = "bcd08a7b-43d2-5ff7-b6d4-c458787f915c" | |||
| SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf" | |||
There was a problem hiding this comment.
I am confused; what does "This changed now" mean? SparseArrays still is listed here (as is Distributed)
| G = Oscar.GAP.Globals | ||
| forGap = Oscar.GAP.julia_to_gap | ||
| fromGap = Oscar.GAP.gap_to_julia |
There was a problem hiding this comment.
Just to say, there are still multiple copies of the above three "aliases", and as I pointed out before, they should all go, as should all uses of Oscar.GAP.julia_to_gap and Oscar.GAP.gap_to_julia. But I am guessing you have this on your TODO and just did not yet get to it.
|
|
||
| function compute(v0, mats, wts::Vector{Vector{Int}}) | ||
| m = length(mats) #--- | ||
| monomials = [nullMon(m)] #--- hier speichern wir die Monombasis. Ein Eintrag steht für Potenzen unserer Basiselemente |
…m/Oscar.jl into bwilop/basisLieHighestWeight
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2115 +/- ##
==========================================
- Coverage 73.43% 0.00% -73.44%
==========================================
Files 367 374 +7
Lines 49945 50935 +990
==========================================
- Hits 36676 0 -36676
- Misses 13269 50935 +37666
|
|
Is there already a plan how to proceed with this PR, in light of the Lie algebra work by @lgoettgens that was already merged? |
|
Besides the initial work in #2145, there is already a plan in #2207 to bring a connection between Oscar and GAP Lie algebras. This would make it possible to, similar to GAPGrroups, handle Oscar Lie algebras in your code and delegate computations to GAP using isomorphisms that will be introduced in #2207. |
|
Okay great. The only part where Lie-Algebras get used is in the file LieAlgebras.jl to call GAP-functions. I think I will wait for #2207 and then use it 👍 . Furthermore, the problems of the PR mentioned so far are fixed and I wanted to ask for another review. I don't understand all of the mathematical background of the project and since answers to my questions were simplified / maybe didn't use the correct vocabulary, the doc of the algorithm basis_lie_highest_weight is still rather short. From my point of view the only relevant algorithm is basis_lie_highest_weight and everything else was only implemented for it and not with the intention to get integrated into Oscar. If there is use for other parts of the code, I can split it up. But maybe it will make more sense to wait with this until everything else Lie-Algebra related is finished. |
|
PR #2207 now has been merged :-) |
|
@BenWilop an I will try to set up a meeting next week to continue working on this. |
|
Closed in favour of Pull request #2402. We opened a new one because of access rights. |
* Code from BasisLieHighestWeight, Pull request #2115 * Removed part of fromGap, gens(ZZx) instaed of x * Simplified paramters with structures and recomputation * TODOs * Added test-files, removed TVec * Removed nullSpace() * VectorSpaceBases-test.jl * NewMonomial-test.jl * compute_sub_weights * Added GAPWrap instead of GAP.Globals where possible * Custom print functions for the structures * tabs to indents * Doctest basis_lie_highest_weight * Changed get_monomial_order_lt to accept name of monomial-order as defined in Oscar.jl * function body of special basis_lie_highest_weight functions * . * DemazureOperators * demazure_operators_summary * oplex_lt * Bugfix ZZ(0) in result instead of ZZ(1) * Comments * Bugfix demazure_operator order of operators * Root Conversion with QQ * RootConversion-test and -data * Bugfix rootconverison A * Bugfix root-conversion D * Bugfix root-conversion F * Bugfix root-conversion E8 * CalculateBetas, CalculateBetas-test * root_to_root_vector * compute_operators_lustzig_nz * Operators as Julia-vectors, find_root_in_chevalley_basis * basis_lie_highest_weight_lustzig * Docstring and adapted methods for lustzig, nz, fflv, string * Print Birational-sequence with alpha_i * Word-calculations only for lustzig * Remove Documenter.jl as dep * Run JuliaFormatter * Fix root conversion * Make tests green * Make RootConversion tests faster * Use lowercase constructors * Refactor root conversion * Remove workaround for old issues * Move some stuff around * Change dynkin type to symbol * Fix doctests * Add basic docs * Excise uses of `gap_to_julia` and `julia_to_gap` * Enhance printing of output * Remove `cache_size` * Remove lots of unused code * Bugfix: Reverse operator list for FFLV * User can input reduced expression through sum of roots instead of searching through GAP list * Refactor polyhedral code * Change `get_lattice_points_of_weightspace` to accept input in alpha_i * Excise all uses of weights in eps_i basis * Change monomial ordering inputs to symbols * `order` -> `ordering` * Refactor passing around lie_algebra * Move user facing functions to separate file * Adapt input to Ghislain's whishes * Make weighted orderings work * Add function to print operators * Replace all uses of `convert` * Refactor algorithm input and some Lie algebra stuff - completely rewrite operator generation (by index, by coeffs of simple roots, lustzig) - more accessors for Lie algebra data - remove some type conversions - rename all vars `lie_algebra` to avoid shadowing of function - Update user functions' arguments to Ghislain's whishes * More random refactoring * More monomial ordering changes * Excise `SparseVectorSpaceBasis` * Fix spelling [skip ci] * `isweighted` -> `_is_weighted` * Fix doctests * Skip sorting of monomials * Fix printing * Fix deprecation warning * Add docstrings to `UserFunctions.jl` * Add tests against Xin's output * Remove unnecessary printings * Add exports * Adjust printing to Ghislain's whishes * Apply suggestions from code review Co-authored-by: Ghislain Fourier <[email protected]> * Fix typo in printing * Fix visibility issue * Add all documented functions to docs --------- Co-authored-by: Ben Wilop <[email protected]> Co-authored-by: Ghislain Fourier <[email protected]>
No description provided.