LieAlgebras: More Lie algebra constructions from/to GAP

[lgoettgens]

Allow the transformation of fin. dim. Lie algebras from/to GAP. This will extend Oscar.iso_oscar_gap and Oscar.iso_gap_oscar.
This enables us to, in the future, use GAP to compute some stuff, similar to the group situation.

[codecov]

Codecov Report

Merging #2207 (2e499c4) into master (708ce15) will increase coverage by 0.27%.
The diff coverage is 92.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2207      +/-   ##
==========================================
+ Coverage   71.62%   71.90%   +0.27%     
==========================================
  Files         377      388      +11     
  Lines       52484    55002    +2518     
==========================================
+ Hits        37593    39547    +1954     
- Misses      14891    15455     +564     

Impacted Files	Coverage Δ
experimental/LieAlgebras/src/LieAlgebra.jl	83.95% <0.00%> (-10.23%)	⬇️
...xperimental/LieAlgebras/test/iso_gap_oscar-test.jl	82.92% <82.92%> (ø)
experimental/LieAlgebras/src/AbstractLieAlgebra.jl	90.47% <100.00%> (+2.67%)	⬆️
experimental/LieAlgebras/src/GapWrapper.jl	100.00% <100.00%> (ø)
experimental/LieAlgebras/src/LinearLieAlgebra.jl	93.22% <100.00%> (+4.64%)	⬆️
experimental/LieAlgebras/src/iso_gap_oscar.jl	100.00% <100.00%> (ø)
experimental/LieAlgebras/src/iso_oscar_gap.jl	100.00% <100.00%> (ø)
...xperimental/LieAlgebras/test/iso_oscar_gap-test.jl	100.00% <100.00%> (ø)
src/GAP/wrappers.jl	92.51% <100.00%> (+0.20%)	⬆️

... and 28 files with indirect coverage changes

[lgoettgens]

I have implemented _iso_oscar_gap and _iso_gap_oscar methods for Lie algebras, similar to the already existing ones in src/GAP/. But I am unsure, where to put them. Since this is part of experimental, I do not know if they should be called from Oscar.iso_oscar_gap and Oscar.iso_gap_oscar and if they should be put into experimental or src/GAP/.

In contrast to the already existing implementations, this has an additional layer of caching. The idea behind that is as follows: I would like all Lie algebras to have the possibility to have a corresponding GAP Lie algebra with the iso as an attribute (-> thus using attribute storage). This would not need the additional layer of caching due to the @attr e.g. here.

Oscar.jl/src/GAP/iso_oscar_gap.jl

Line 386 in be51cf8

@attr Map function iso_oscar_gap(F)

For the converse direction (GAP -> Oscar), I would like the codomain LO of the return value of _iso_gap_oscar(LG) to know about the already created isomorphic GAP object LG and the iso (which is just the inverse of the return value). This for me doesn't seem possible by hacking into the cache used by @attr without relying on the way how the key gets created inside AbstractAlgebra. Ideas on how to handle that in a better way are welcome.

[lgoettgens]

This is ready to review, but NOT ready to merge yet. The decision on how to integrate _iso_oscar_gap and _iso_gap_oscar has to be done and implemented first.
For a review, I would ping @fingolfin and @ThomasBreuer. (I unfortunately cannot use the gh tool to ask for a review.)

[lgoettgens]

Rebased on top of #2262 to avoid conflicts.

[fingolfin]

The decision on how to integrate _iso_oscar_gap and _iso_gap_oscar has to be done and implemented first.

I've read through what you've written several times now, and I honestly don't understand what the problem is / which decision has to be made. The only thing I see listed as needing any kind of work or decision is this:

I would like the codomain LO of the return value of _iso_gap_oscar(LG) to know about the already created isomorphic GAP object LG and the iso (which is just the inverse of the return value).

But I don't understand what the problem there is, isn't this just a matter of calling set_attribute! on the codomain (which is an attribute storing Julia object, after all) to store the iso you just constructed?

Isn't it more the converse which is a problem: namely, ensure that the domain of _iso_gap_oscar (which is a GAP object) only ever gets assigned a single unique Julia object (and then likewise for the codomain of _iso_oscar_gap)? To solve that, one would have to store the isomorphism inside the GAP object (which is no problem for those which are attribute storing in the GAP sense, which is related to but different from the OSCAR attribute storage system)

[lgoettgens]

The decision on how to integrate _iso_oscar_gap and _iso_gap_oscar has to be done and implemented first.

I've read through what you've written several times now, and I honestly don't understand what the problem is / which decision has to be made.

I should have written a bit more about that, sorry. To work as expected, the methods _iso_oscar_gap and _iso_gap_oscar introduced in this PR would have to be hooked up into src/GAP/iso_gap_oscar.jl and src/GAP/iso_oscar_gap.jl, e.g. called from

Oscar.jl/src/GAP/iso_gap_oscar.jl

Line 34 in e926036

function _iso_gap_oscar(F::GAP.GapObj)

This would add experimental/LieAlgebras/ as a dependency of src/GAP/. Moving the functionality from this PR into the two files in src/GAP/ would make this much more complicated to de-tangle if ever wished so in the future. Not moving it, would make the inclusion order in Oscar.jl much more complicated. In particular, experimental/LieAlgebras/ would have to be included in-between different files from src/GAP/ (at least in my thoughts).

I hope the above helps a bit to understand my problems and thought process.

I would like the codomain LO of the return value of _iso_gap_oscar(LG) to know about the already created isomorphic GAP object LG and the iso (which is just the inverse of the return value).

But I don't understand what the problem there is, isn't this just a matter of calling set_attribute! on the codomain (which is an attribute storing Julia object, after all) to store the iso you just constructed?

There is no problem per se. I just added some justification on why I used get_attribute/set_attribute! instead of a simpler @attr.

Isn't it more the converse which is a problem: namely, ensure that the domain of _iso_gap_oscar (which is a GAP object) only ever gets assigned a single unique Julia object (and then likewise for the codomain of _iso_oscar_gap)? To solve that, one would have to store the isomorphism inside the GAP object (which is no problem for those which are attribute storing in the GAP sense, which is related to but different from the OSCAR attribute storage system)

Yes, indeed. However, I would not see that as a problem for iso_oscar_gap and iso_gap_oscar for Lie algebras, but for other types as well. Thus, I would propose a general solution in src/GAP/ for this.

[fingolfin]

To work as expected, the methods _iso_oscar_gap and _iso_gap_oscar introduced in this PR would have to be hooked up into src/GAP/iso_gap_oscar.jl and src/GAP/iso_oscar_gap.jl,

Could you please elaborate? I.e. what expectation does this meet? Resp. what undesirable things happen if we don't "hook it up"?

[ThomasBreuer]

Currently iso_gap_oscar(D::GapObj) is one Julia function that checks the GAP type of D and then decides which Julia function will do the work. In order to be able to add more possible D in Oscar.jl/experimental code (or in private code), one possibility would be to delegate to GAP's method selection; then the GAP methods would call the Julia functions in question. I can provide a pull request for that.

[lgoettgens]

In order to be able to add more possible D in Oscar.jl/experimental code (or in private code), one possibility would be to delegate to GAP's method selection; then the GAP methods would call the Julia functions in question. I can provide a pull request for that.

That sound like a good approach. Thanks for working on that @ThomasBreuer!

[lgoettgens]

For my convenience, this now includes the changes of #2309.

[lgoettgens]

@fingolfin I guess you meant something like this in the last paragraph of #2207 (comment)?
It's not working right now. To remember a future pr, I added it in a comment for now.

[lgoettgens]

This pr now allows the use of the "usual" functions Oscar.iso_gap_oscar and Oscar.iso_oscar_gap for Lie algebras as well. From my pov, this is ready to go now (it has already been rebased on master after the merge of #2309).

[ThomasBreuer]

I have added some minor comments.

[lgoettgens]

Is this PR still waiting for something?

[ThomasBreuer]

@lgoettgens No. Sorry for not merging it earlier.