LieAlgebras: Add docs

[lgoettgens]

This PR adds documentation to the LieAlgebras folder in experimental.

Weirdly, some docstrings will not show up in the HTML, e.g. (::LieAlgebra{C})(::Vector{C}) where {C<:RingElement}.
Can someone help me with that?

Edit: Additionally some minor changes.

[codecov]

Codecov Report

Merging #2425 (690e14e) into master (3b9ed2b) will increase coverage by 0.00%.
The diff coverage is 89.47%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2425    +/-   ##
========================================
  Coverage   71.65%   71.66%            
========================================
  Files         392      395     +3     
  Lines       52831    53015   +184     
========================================
+ Hits        37855    37991   +136     
- Misses      14976    15024    +48     

Impacted Files	Coverage Δ
experimental/LieAlgebras/src/Util.jl	100.00% <ø> (+36.66%)	⬆️
experimental/LieAlgebras/src/LieAlgebra.jl	82.35% <75.00%> (-1.60%)	⬇️
experimental/LieAlgebras/src/LieAlgebraModule.jl	95.35% <92.85%> (+0.05%)	⬆️
experimental/LieAlgebras/src/AbstractLieAlgebra.jl	90.47% <100.00%> (ø)
experimental/LieAlgebras/src/LinearLieAlgebra.jl	93.22% <100.00%> (ø)

... and 24 files with indirect coverage changes

[fingolfin]

Thank you, great improvement!

I have some suggestions but mostly optional and/or could be done later. Also the point on base_ring / coefficient_ring of course need not be addressed in this PR, but we should still address them (so either do so, or if you don't have time right now, perhaps you can open an issue for them?)

[fingolfin]

Not about this PR but about existing code: I find this surprising, I would have expected base_ring to return the Lie algebra. But instead it seems to return the base ring of the Lie algebra, i.e. its "coefficient ring" or "left-acting domain" (in GAP parlance). Of course in part this is because base_ring is notoriously ill-defined, so guessing what it does is always problematic.

And right now, this definition is inconsistent with other existing module types, where the full acting ring is returned.

So this is a bit unfortunate. One solution then would be to change base_ring(V::LieAlgebraModule) to return the same as base_lie_algebra(V), and then add a separate function to access the "coefficients domain".

E.g. under the name coefficient_ring; while it is not super common to refer to the R in R-algebra by the name coefficient ring, I think it is fine as long as we specify it explicitly:

The coefficient ring of a module is the coefficient ring of its base ring.
The coefficient ring of a ring which is constructed as an R-algebra is R.

The bit about "constructed as" is meant to clarify that while e.g. the polynomial ring GF(4)[:x] is both an $F_4$ and an $F_2$ algebra, it was constructed as an $F_4$ algebra.

After typing this I remembered we have #1505 (and also #938) ... so there :-)

[lgoettgens]

I understand your point.

However, I found it quite confusing to refer to the lie algebra as base_ring, although it is not a ring. My idea now would be to abolish base_ring, as it is misleading and instead use base_lie_algebra and coefficient_ring.
But that will happen in a future PR.

[fingolfin]

Well, every Lie algebra is also a Lie ring, and many people refer to "non-associative rings". But yeah, a more generic name than base_ring would be helpful...

But in general my view remains that I'd wish we could do completely away with base_ring ;-). And if we have coefficient_ring and base_lie_algebra (or, for that matter, just lie_algebra(M) ???), it's not so important what base_ring does exactly (or we could even consider omitting it entirely?).

[thofma]

I think for the name of genericity (is this a word), the name should not contain "lie". For the modules over finite-dimensional associative algebras, I called this just algebra(M) and coefficient_ring(M) == coefficient_ring(algebra(M)) (there is no base_ring(M)).

julia> A = matrix_algebra(QQ, 2);

julia> V = Hecke.Amodule(A, [matrix(a) for a in basis(A)]);

julia> algebra(V)
Matrix algebra of dimension 4 over Rational field

julia> coefficient_ring(V)
Rational field

Not sure about the coefficient_ring anymore, but I like algebra(M). I think it would be great if we could try to be somehow consistent. (Sorry about the missing documentation.) I don't insist that the names I have chosen are the best ones and I am happy to adjust them.

[lgoettgens]

With algebra, I have kind of the same problem as with base_ring for denoting the Lie algebra, as it is not an algebra in the usual sense.

[thofma]

We just relax what we mean with an "algebra", so it could be fine. I don't think this would be a big problem. But we will discuss this later.

[fingolfin]

Again off-topic, but: you can do away with C in many cases, making the code maybe more readable?

Suggested change

	dim(V::LieAlgebraModule{C}) -> Int
	Return the dimension of the Lie algebra module `V`.
	"""
	dim(V::LieAlgebraModule{C}) where {C<:RingElement} = V.dim
	dim(V::LieAlgebraModule) -> Int
	Return the dimension of the Lie algebra module `V`.
	"""
	dim(V::LieAlgebraModule) = V.dim

[lgoettgens]

Didn't know that. I will look into it for a future PR.

[lgoettgens]

Thanks for your comments @fingolfin!
I hope I addressed all of them.

Unfortunately, the docs are not building. Do you have any idea why?

[fingolfin]

There still are errors in your docs:

┌ Error: unable to get the binding for '(::LinearLieAlgebra{C})(::MatElem{C}) where {C<:RingElement}' in `@docs` block in src/Experimental/LieAlgebras/lie_algebras.md:31-36 from expression ':((::LinearLieAlgebra{C})(::MatElem{C}) where C <: RingElement)' in module Oscar
│ ```@docs
│ matrix_repr_basis(::LinearLieAlgebra{C}) where {C<:RingElement}
│ matrix_repr_basis(::LinearLieAlgebra{C}, ::Int) where {C<:RingElement}
│ matrix_repr(::LinearLieAlgebraElem{C}) where {C<:RingElement}
│ (::LinearLieAlgebra{C})(::MatElem{C}) where {C<:RingElement}
│ ```
│   exception = `binding` cannot understand expression `::LinearLieAlgebra{C}`.
└ @ Documenter.Expanders ~/.julia/packages/Documenter/H5y27/src/Utilities/Utilities.jl:32

I am afraid documenting "call syntax" is simply not supported at this point, see JuliaDocs/Documenter.jl#558

[lgoettgens]

That's unfortunate. I guess I then just copy the docstrings into the corresponding .md file for now.

[lgoettgens]

Everything builds now fine. From my point, this is good to go (once the CI finishes).

The two remaining conversations (on base_ring and removing {C}) will be postponed to a future PR.

[lgoettgens]

Is there something that needs to be changed about this @fingolfin? Otherwise, I would appreciate a merge and I will work on the things brought up here in a follow-up PR.

[fingolfin]

Oops, sorry, I forgot about this :-(