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

Add serialization upgrade script for type renaming #3303

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Feb 1, 2024

This approaches to add an upgrade script for serialized files incorporating the renamings from #3231 and #3288. A script for applying the renaming can is available in #3288 (comment).

I looked through all occurrences of @register_serialization_type in the codebase and compared them with the renaming script. If the type got renamed, both old and new names got added to the list in the serialization code.

Everything got a bit finicky, as type strings can be at a lot of different places, and furthermore inside :_refs and this needs to get reloaded after the upgrading into the DeserializationState.

I reverted to data/JohnsonMatrices/ folder to before #3288. They should get either kept at this older state or upgraded once this PR works using

for index in keys(Oscar._johnson_names)
       println(index)
       loaded = load(joinpath(Oscar.oscardir, "data", "JohnsonMatrices", string("j", index, ".mat")))
       save(joinpath(Oscar.oscardir, "data", "JohnsonMatrices", string("j", index, ".mat")), loaded)
end

cc @thofma @joschmitt @antonydellavecchia

@lgoettgens
Copy link
Member Author

Ok, so this works now locally with some examples that I serialized on 56feead (i.e. before #3231) and loaded them on this branch.
Furthermore, loading all Johnson matrices in data/JohnsonMatrices/ works now locally (and should be verified by the CI job testing johnson_solid).

@antonydellavecchia can you please have a look if what I've done here looks more or less sensible? You don't have to check the concrete types to update, just the general framework of renaming types. Thanks!

@lgoettgens lgoettgens marked this pull request as ready for review February 2, 2024 10:12
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.

Things look more or less correct. If tests are passing then great. I was a bit concerned about the fact you need to update s.refs after the upgrade, but this is probably something for me to look into. Thanks for doing this!

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

Merging #3303 (a103b6c) into master (757489f) will increase coverage by 39.94%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3303       +/-   ##
===========================================
+ Coverage   41.65%   81.60%   +39.94%     
===========================================
  Files         533      547       +14     
  Lines       71640    73530     +1890     
===========================================
+ Hits        29844    60005    +30161     
+ Misses      41796    13525    -28271     
Files Coverage Δ
src/Serialization/Fields.jl 93.96% <100.00%> (+92.83%) ⬆️
src/Serialization/Rings.jl 91.53% <ø> (+77.32%) ⬆️
src/Serialization/Upgrades/0.15.0.jl 100.00% <100.00%> (ø)
src/Serialization/Upgrades/main.jl 100.00% <ø> (+94.44%) ⬆️
src/Serialization/main.jl 85.89% <100.00%> (+17.53%) ⬆️

... and 407 files with indirect coverage changes

@thofma thofma merged commit a65ac09 into oscar-system:master Feb 2, 2024
23 checks passed
@lgoettgens lgoettgens deleted the lg/serialization_upgrade branch February 2, 2024 17:42
ooinaruhugh pushed a commit to ooinaruhugh/Oscar.jl that referenced this pull request Feb 15, 2024
* Revert JohnsonMatrices to an older state

* Reload refs after upgrade script

* Add serialization upgrade script for type renaming
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.

3 participants