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

Move LieTheory serialization to src; add WeightLattice serialization #4414

Conversation

lgoettgens
Copy link
Member

This part was not moved as part of #4399.

Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.38%. Comparing base (cd79a0b) to head (301950c).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4414      +/-   ##
==========================================
- Coverage   84.38%   84.38%   -0.01%     
==========================================
  Files         658      658              
  Lines       87157    87133      -24     
==========================================
- Hits        73551    73530      -21     
+ Misses      13606    13603       -3     
Files with missing lines Coverage Δ
experimental/LieAlgebras/src/serialization.jl 100.00% <ø> (ø)
experimental/LieAlgebras/test/WeylGroup-test.jl 100.00% <ø> (+3.12%) ⬆️
src/Serialization/LieTheory.jl 100.00% <100.00%> (ø)
src/Serialization/main.jl 85.27% <ø> (ø)

... and 1 file with indirect coverage changes


function save_object(s::SerializerState, P::WeightLattice)
save_data_dict(s) do
save_typed_object(s, root_system(P), :root_system)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the type information for the root system will be moved out during the upcoming refactor,
If you can rewrite this so that Weight Lattice uses params, this will make the upgrade simple / virtually nothing (as in the file wont change just the code)

end
end

function load_object(s::DeserializerState, ::Type{WeightLattice})
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would then need to accept root system as the third parameter

end

function load_object(s::DeserializerState, ::Type{WeylGroup})
R = load_typed_object(s, :root_system)
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly here, however since we could already save this, I think it's best to leave this to the refactor.

Copy link
Collaborator

@antonydellavecchia antonydellavecchia left a comment

Choose a reason for hiding this comment

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

Looks good, see comments.

@lgoettgens
Copy link
Member Author

Since WeylGroup was already there, I thought it would be easiest to add WeightLattice to be completely analogous (aka to then adapt both in exactly the same way during the refactor). But if you prefer, I could try to adapt the WeightLattice functions according to your comments (but in this way making the two implementations differ).
I would leave it for you (@antonydellavecchia ) to decide

@antonydellavecchia
Copy link
Collaborator

Probably won't make too much of a difference, so I'll just merge

@antonydellavecchia antonydellavecchia merged commit 3dd332d into oscar-system:master Jan 6, 2025
29 of 30 checks passed
@lgoettgens lgoettgens deleted the lg/root-system-serialization branch January 6, 2025 13:05
@lgoettgens
Copy link
Member Author

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants