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

Serialization of Lie algebras #3980

Merged
merged 20 commits into from
Aug 22, 2024
Merged

Conversation

antonydellavecchia
Copy link
Collaborator

Implements serialization for Lie Algebras

@lgoettgens lgoettgens added topic: LieAlgebras experimental Only changes experimental parts of the code and removed experimental Only changes experimental parts of the code labels Jul 29, 2024
@lgoettgens
Copy link
Member

lgoettgens commented Jul 29, 2024

@antonydellavecchia I started duplicating everything that you added for the other two types of Lie algebras. All of them now have different problems I am afraid I need your help with.

  1. AbstractLieAlgebra
    needs serialization of SRow
    example:
L = lie_algebra(QQ, :A, 2)
  1. LinearLieAlgebra
    saving and loading works, but it yields identical (===) objects. And it seems that it does not call the specific load function at all, as loading fails after calling Oscar.reset_global_serializer_state() with some problem with deserializing matrices.
    example:
L = general_linear_lie_algebra(QQ, 2)
  1. DirectSumLieAlgebra
    examples:
# empty direct sum
L = direct_sum(QQ, LieAlgebra{QQFieldElem}[])

# non-trivial example
L1 = general_linear_lie_algebra(QQ, 2)
L2 = lie_algebra(QQ, :A, 2)
L = direct_sum(L1, L1, L2) # it is important that the first two summands are still identical after saving and loading
  1. Element types for all of these three
    but they are essentially the same, as they just contain a parent pointer and a coeff vector (or rather a 1xn matrix)
    examples (for all L as above):
x = basis(L, 1) # for one elem
xs = basis(L) # for a vector of elems
  1. Testing
    How do I actually test if saving and loading are correct, if they give me the identical (===) object? I rather just compare some properties between original and deserialized.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 98.31461% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.64%. Comparing base (61bf7dc) to head (863cc95).
Report is 1 commits behind head on master.

Files Patch % Lines
experimental/LieAlgebras/test/RootSystem-test.jl 90.90% 1 Missing ⚠️
experimental/LieAlgebras/test/WeylGroup-test.jl 96.66% 1 Missing ⚠️
experimental/LieAlgebras/test/setup_tests.jl 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3980      +/-   ##
==========================================
+ Coverage   84.58%   84.64%   +0.05%     
==========================================
  Files         597      598       +1     
  Lines       82265    82445     +180     
==========================================
+ Hits        69587    69783     +196     
+ Misses      12678    12662      -16     
Files Coverage Δ
experimental/LieAlgebras/src/LieAlgebras.jl 100.00% <ø> (ø)
experimental/LieAlgebras/src/serialization.jl 100.00% <100.00%> (ø)
experimental/LieAlgebras/test/RootSystem-test.jl 98.52% <90.90%> (-1.48%) ⬇️
experimental/LieAlgebras/test/WeylGroup-test.jl 97.77% <96.66%> (-2.23%) ⬇️
experimental/LieAlgebras/test/setup_tests.jl 95.18% <94.44%> (-0.09%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

@antonydellavecchia Thanks a lot for your help with this! I added some comments to things that came up after our discussion when I played around some more.
The problem with empty vectors is worse than expected, see #3983 for details what I found while digging around.
Can you take it from here? Once you are happy with all the changes in this PR, please write your approval in a comment (since Github won't allow you to do it the usual way) and I am then happy to merge.

save_object(s, get_attribute(L, symbol_prop), symbol_prop)
end
end
# TODO: handle root_system
Copy link
Member

Choose a reason for hiding this comment

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

@HereAround @emikelsons please wait for this TODO to be resolved (ETA end of August) before putting Lie algebras inside your FTheoryTools serialization. Or, alternatively, have a script that re-creates the serialized models so that we can just re-run it once the serialization of Lie algebras changes

Copy link
Member

Choose a reason for hiding this comment

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

Edit: I already managed to resolve this today. But you should have a reliable way to re-generate your FTheoryTools models anyway, since there certainly will be changes to the Lie algebra serialization. (Not necessarily breaking, but adding more information about them and thus e.g. speeding up some operations on them)

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, this slipped my attention.

All good. I have opened #4008 to coordinate this and many other serialization changes for FTheoryTools. In other words, waiting for you and @antonydellavecchia to tell us when/how to proceed.

src/Serialization/containers.jl Outdated Show resolved Hide resolved
src/Serialization/main.jl Outdated Show resolved Hide resolved
src/Serialization/main.jl Outdated Show resolved Hide resolved
src/Serialization/serializers.jl Outdated Show resolved Hide resolved
@lgoettgens lgoettgens changed the title Serialization of Lie Algebra Serialization of Lie algebras Jul 30, 2024
@lgoettgens lgoettgens marked this pull request as ready for review July 30, 2024 20:19
@lgoettgens
Copy link
Member

I added serialization for some more Lie algebra related types. This is starting to make fun 😅
@antonydellavecchia could you have a look at the new implementations as well? They seem to work, but maybe I did something too hacky somewhere.

@lgoettgens

This comment was marked as outdated.

@lgoettgens
Copy link
Member

#4020 looks really interesting, but will introduce conflicts. Thus I suggest we wait with this until #4020 is merged. To nevertheless get the bugfixes etc. from this PR in master in a timely manner, I extracted them to #4021

@lgoettgens
Copy link
Member

This is now rebased on top of #4020. Once that and #4030 are merged, I can simplify this PR a bit

@lgoettgens lgoettgens force-pushed the adv/serialize-lie-algebra branch 3 times, most recently from 8c14f0f to 6ca22ee Compare August 21, 2024 13:21
@lgoettgens lgoettgens marked this pull request as ready for review August 22, 2024 15:41
@lgoettgens lgoettgens added the experimental Only changes experimental parts of the code label Aug 22, 2024
Copy link
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

This is now finished from my POV. @antonydellavecchia could you please have a look if you see any issues and if not, proceed with the merge?

@antonydellavecchia antonydellavecchia merged commit f7601d3 into master Aug 22, 2024
29 checks passed
@antonydellavecchia antonydellavecchia deleted the adv/serialize-lie-algebra branch August 22, 2024 19:52
HechtiDerLachs pushed a commit to HechtiDerLachs/Oscar.jl that referenced this pull request Sep 13, 2024
* some progress on lie algebra serialization

* more progress

* serialization for SRow

* more

* fix for general lie algebras

* direct sum lie algebras working

* remove whitespace

* Fix for empty direct sum

* Make LieAlgebraElem work

* Some optimizations

* Add serialization tests

* Add serialization of attributes

* Add macro for importing serialization functions

* Add reference to issue

* foo

* Add serialization of root systems

* Add serialization of weyl groups

* Connect Lie algebra and root system serialization

* Make attribute serialization work for Lie algebras

* Add artifact serialization test

---------

Co-authored-by: Lars Göttgens <lars.goettgens@rwth-aachen.de>
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 serialization topic: LieAlgebras
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants