Make Lie algebra construction faster #3937

lgoettgens · 2024-07-15T10:27:04Z

Follow-up to #3831 that implements some performance improvements and fixes some type instabilities and other issues reported by JET.

The following diagram shows the enormous improvement. In particular, we are now almost a third faster than GAP for all of these test cases.

created using:

cases=[(:A, 1), (:A, 2), (:A, 3), (:A, 4), (:A, 5), (:A, 6), (:A, 7), (:A, 8), (:B, 2), (:B, 3), (:B, 4), (:B, 5), (:B, 6), (:B, 7), (:B, 8), (:D, 4), (:D, 5), (:D, 6), (:D, 7), (:D, 8), (:E, 6), (:E, 7), (:E, 8), (:F, 4), (:G, 2)]
gaptimes = [begin @info case; @belapsed GAP.Globals.SimpleLieAlgebra($(GAP.Obj(String(case[1]))), $(case[2]), $(GAP.Globals.Rationals)) end for case in cases]
oscartimes = [begin @info case; @belapsed lie_algebra($QQ, $(case[1]), $(case[2])) end for case in cases]

using StatsPlots, Plots

groupedbar([[1 for _ in cases] (oscartimes_master./gaptimes) (oscartimes_5b6483./gaptimes)]; xticks=(eachindex(cases), join.(cases)),label=["GAP (normalized)" "OSCAR (master)" "OSCAR (PR)"],bar_width=0.65,ylims=(-Inf,4))

lgoettgens · 2024-07-15T10:27:52Z

experimental/LieAlgebras/src/RootSystem.jl

@@ -21,7 +23,13 @@

    R = new(mat)
    R.positive_roots = map(r -> RootSpaceElem(R, r), pos_roots)
+    R.positive_roots_map = Dict(
+      (coefficients(root), ind) for (ind, root) in enumerate(R.positive_roots::Vector{RootSpaceElem})
+    )


pre-computing this and using this in is_root instead of a linear search is the main speed improvement of this PR

lgoettgens · 2024-07-15T10:28:55Z

experimental/LieAlgebras/src/RootSystem.jl

@@ -438,13 +450,13 @@ function Base.:(*)(q::RationalUnion, r::RootSpaceElem)
 end

 function Base.:(+)(r::RootSpaceElem, r2::RootSpaceElem)
-  @req root_system(r) === root_system(r2) "$r and $r2 must belong to the same root space"
+  @req root_system(r) === root_system(r2) "parent root system mismatch"


these strings get interpolated even in the case that no error is thrown. Withouth string interpolation, there is basically no overhead

joschmitt · 2024-07-15T10:36:53Z

experimental/LieAlgebras/src/RootSystem.jl

@@ -104,6 +114,10 @@ end
  return cartan_bilinear_form(cartan_matrix(R); check=false)
 end

+@attr QQMatrix function bilinear_form_QQ(R::RootSystem)


I think something like

Suggested change

@attr QQMatrix function bilinear_form_QQ(R::RootSystem)

@attr QQMatrix function bilinear_form(::QQField, R::RootSystem)

would be more OSCAR-y?

Yeah, but this does not allow @attr to work. Furthermore, this is just supposed to be internal

I renamed it now to _bilinear_form_QQ to make it more visible that its internal

codecov · 2024-07-15T11:22:07Z

Codecov Report

Attention: Patch coverage is 92.70833% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.00%. Comparing base (810a77d) to head (d88ff62).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3937      +/-   ##
==========================================
+ Coverage   83.99%   84.00%   +0.01%     
==========================================
  Files         591      591              
  Lines       81387    81440      +53     
==========================================
+ Hits        68360    68417      +57     
+ Misses      13027    13023       -4

Files	Coverage Δ
experimental/LieAlgebras/src/CartanMatrix.jl	`97.30% <100.00%> (ø)`
experimental/LieAlgebras/src/GapWrapper.jl	`100.00% <100.00%> (ø)`
experimental/LieAlgebras/src/LieAlgebraModule.jl	`90.54% <100.00%> (ø)`
experimental/LieAlgebras/src/LieAlgebras.jl	`100.00% <ø> (ø)`
experimental/LieAlgebras/src/iso_gap_oscar.jl	`100.00% <100.00%> (ø)`
...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/src/AbstractLieAlgebra.jl	`87.24% <91.66%> (+0.06%)`	⬆️
experimental/LieAlgebras/src/RootSystem.jl	`80.95% <89.28%> (+9.23%)`	⬆️

... and 1 file with indirect coverage changes

felix-roehrich

Looks good, thanks.

fingolfin · 2024-07-19T13:36:47Z

experimental/LieAlgebras/src/RootSystem.jl

@@ -274,7 +288,7 @@ This is a more efficient version for `positive_coroots(R)[i]`.
 Also see: `positive_coroots`.
 """
 function positive_coroot(R::RootSystem, i::Int)
-  return R.positive_coroots[i]::DualRootSpaceElem
+  return (R.positive_coroots::Vector{DualRootSpaceElem})[i]


How about this to avoid the repeated type annotations? (But I am fine with this either way)

Suggested change

return (R.positive_coroots::Vector{DualRootSpaceElem})[i]

return positive_coroots(R)[i]

for the positive (co)roots this indeed works. For the negative ones, however, this is not optimal as it would first negate all positive roots to then only take one of them. And I rather have positive and negative consistent here.

fingolfin · 2024-07-19T13:39:38Z

experimental/LieAlgebras/src/RootSystem.jl

+  i = get(root_system(r).positive_coroots_map, coefficients(r), nothing)
  if isnothing(i)
    return false, 0
  else
    return true, i
  end


Obv. this is existing code, and it is fine, but just as food for thought:

Suggested change

i = get(root_system(r).positive_coroots_map, coefficients(r), nothing)

if isnothing(i)

return false, 0

else

return true, i

end

i = get(root_system(r).positive_coroots_map, coefficients(r), 0)

return !is_zero(i), i

will be included in the next round of refactoring :)

lgoettgens added topic: LieTheory experimental Only changes experimental parts of the code labels Jul 15, 2024

lgoettgens requested review from fingolfin and felix-roehrich July 15, 2024 10:27

lgoettgens commented Jul 15, 2024

View reviewed changes

joschmitt reviewed Jul 15, 2024

View reviewed changes

lgoettgens force-pushed the lg/Lie-algebra-faster branch from d88ff62 to da9ffd0 Compare July 15, 2024 12:37

lgoettgens added 6 commits July 15, 2024 14:38

Make root lookup in root systems faster

7adb888

Enhance inference for SRow in AbstractLieAlgebra

90861ff

Use faster root lookup in more places

3d89039

Cache QQMatrix(bilinear_form(::RootSystem))

962e8f8

Make some more code type stable / fix JET issues

184d11d

Use a view

040d95a

lgoettgens force-pushed the lg/Lie-algebra-faster branch from da9ffd0 to a5298fe Compare July 15, 2024 13:34

formatting

a5298fe

felix-roehrich approved these changes Jul 16, 2024

View reviewed changes

fingolfin approved these changes Jul 19, 2024

View reviewed changes

lgoettgens merged commit edb44e2 into oscar-system:master Jul 19, 2024
26 checks passed

lgoettgens deleted the lg/Lie-algebra-faster branch July 19, 2024 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Lie algebra construction faster #3937

Make Lie algebra construction faster #3937

lgoettgens commented Jul 15, 2024

lgoettgens Jul 15, 2024

lgoettgens Jul 15, 2024

joschmitt Jul 15, 2024

lgoettgens Jul 15, 2024

lgoettgens Jul 15, 2024

codecov bot commented Jul 15, 2024 •

edited

Loading

felix-roehrich left a comment

fingolfin Jul 19, 2024

lgoettgens Jul 19, 2024

fingolfin Jul 19, 2024

lgoettgens Jul 19, 2024

	@attr QQMatrix function bilinear_form_QQ(R::RootSystem)
	@attr QQMatrix function bilinear_form(::QQField, R::RootSystem)

	return (R.positive_coroots::Vector{DualRootSpaceElem})[i]
	return positive_coroots(R)[i]

Make Lie algebra construction faster #3937

Make Lie algebra construction faster #3937

Conversation

lgoettgens commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2024 • edited Loading

Codecov Report

felix-roehrich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 15, 2024 •

edited

Loading