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 Refactor #4162

Draft
wants to merge 60 commits into
base: master
Choose a base branch
from
Draft

Serialization Refactor #4162

wants to merge 60 commits into from

Conversation

antonydellavecchia
Copy link
Collaborator

Refactor serialization to use a new function called type_params which helps build a stack of type dependencies which will allow for serializing type information before the data.

lkastner
lkastner previously approved these changes Nov 20, 2024
@ThomasBreuer
Copy link
Member

ThomasBreuer commented Jan 20, 2025

Please see the changes I've just pushed to get the GAP object serialization working

Thanks @antonydellavecchia.
I am going to adjust the serialization for GAP pc groups.
Now the tests for GAP pc groups pass.

@@ -112,36 +93,27 @@ end

@register_serialization_type WeylGroup uses_id

type_params(W::WeylGroup) = WeylGroup, root_system(W)
Copy link
Member

Choose a reason for hiding this comment

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

Stupid question maybe: It seems to me that the first return value of type_params is always the argument type. Is this a correct observation?
If yes, wouldn't it be possible to refactor that away into the calling function, so that type_params loses the first return value? Imo, this could make the code a bit more readable. If there is a reason for this, I am happy to learn about it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe there is a way to get a round it, but it was the only way I found I could get the new save_type_params to work. See main.jl

save_data_dict(s, :attrs) do
for attr in attrs_list(s, T)
has_attribute(obj, attr) && save_typed_object(s, get_attribute(obj, attr), attr)
save_type_params(s::SerializerState, obj::Any) = save_type_params(s, type_params(obj)...)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
save_type_params(s::SerializerState, obj::Any) = save_type_params(s, type_params(obj)...)
save_type_params(s::SerializerState, obj::Any) = save_type_params(s, typeof(obj), type_params(obj)...)

I think this could do the trick

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes this is probably fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually, a little trickier than expected, there is an issue with the containers.

I'll have to dig deeper, but I dont have time now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've pushed a fix for this now

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.

6 participants