Skip to content

Fix deserialization of MatGroup#5883

Merged
benlorenz merged 8 commits intooscar-system:masterfrom
lgoettgens:lg/upgrade-MatGroup-serialization
Mar 24, 2026
Merged

Fix deserialization of MatGroup#5883
benlorenz merged 8 commits intooscar-system:masterfrom
lgoettgens:lg/upgrade-MatGroup-serialization

Conversation

@lgoettgens
Copy link
Copy Markdown
Member

Resolves #5882 by adding an upgrade script.

I am not quite sure if I assembled the upgrade script in the right way, as there was no pure renaming since we have upgrade_recursive. In particular, I am not sure if upgrade_recursive needs to be called.

As the corresponding renaming was already released with Oscar 1.7.0, IMO we should backport this asap. (cc @benlorenz @aaruni96 )

@lgoettgens lgoettgens added serialization release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes bug: unexpected error backport 1.7.x labels Mar 20, 2026
Comment thread src/Serialization/Upgrades/1.7.0.jl Outdated
@lgoettgens
Copy link
Copy Markdown
Member Author

lgoettgens commented Mar 20, 2026

Ok, so it seems that rename_types does not work anymore for recent-ish upgrades... @antonydellavecchia could you take this from here?

ThomasBreuer and others added 4 commits March 20, 2026 16:58
The update sript for version 1.4.0 can create dictionaries
which have a `:name` but no `:params`,
The function `rename_types` was not aware of that.
I don't see why this would be necessary, the type names should be in the _type / params
this didn't break loading but doesn't work well with subsequent upgrades
@benlorenz
Copy link
Copy Markdown
Member

I added three examples here: oscar-system/serialization-upgrade-tests#13
One of these is from the leech pairs database, the other two are saved with Oscar 1.6.0.

I set DO NOT MERGE to make sure this is not merged before the test files.

@ThomasBreuer
Copy link
Copy Markdown
Member

Concerning the commit "dont recurse into :data section for type rename":
I had copied the code from the update script 1.2.0.jl, the "youngest" such script that involves rename_types.
So either the recursion was already unnecessary in that script, or the data format has been changed since then.

avoid using leech pairs from experimental
@benlorenz
Copy link
Copy Markdown
Member

benlorenz commented Mar 23, 2026

Concerning the commit "dont recurse into :data section for type rename": I had copied the code from the update script 1.2.0.jl, the "youngest" such script that involves rename_types. So either the recursion was already unnecessary in that script, or the data format has been changed since then.

In theory, types names should not be within the data section, but we had some cases where this was violated. There were some fixes regarding this at some point but I don't remember when exactly.
So it is possible that this was not necessary but it is also possible that this was fixed after 1.2.
But I think it is more likely that this block was never necessary, removing it (from the 1.2 upgrade script) doesn't seem to break anything. But still, to be safe I would just keep it as it is.

@lgoettgens
Copy link
Copy Markdown
Member Author

Thanks @benlorenz! I would approve, but that's not possible because this is formally my own PR. Once the hash has been updated to the merge commit of oscar-system/serialization-upgrade-tests#13, this is good to go from my POV.

@benlorenz benlorenz merged commit 9149efc into oscar-system:master Mar 24, 2026
35 of 36 checks passed
benlorenz added a commit that referenced this pull request Mar 24, 2026
Co-authored-by: ThomasBreuer <sam@math.rwth-aachen.de>
Co-authored-by: Benjamin Lorenz <lorenz@math.tu-berlin.de>
(cherry picked from commit 9149efc)
Kazak-11 pushed a commit to Kazak-11/Oscar.jl that referenced this pull request Apr 2, 2026
Co-authored-by: ThomasBreuer <sam@math.rwth-aachen.de>
Co-authored-by: Benjamin Lorenz <lorenz@math.tu-berlin.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.7.x done bug: unexpected error release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes serialization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot load MatrixGroup serialized before renaming

5 participants