Serialization Refactor #4162

antonydellavecchia · 2024-09-28T20:31:55Z

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.

lgoettgens · 2025-01-15T09:13:21Z

This PR seems to have diverged a lot from current master due to other changes to the serialization code in the meantime. Could you try to resolve the conflicts / rebase this onto a more current master, so that one can more easily follow what's happening and CI runs again?

I am a bit afraid that this just continues to diverge even further in the future, which makes it harder to resolve conflicts once you want to merge it.

many changes since approval, and PR has conflicts. please re-review once this is ready to merge

lgoettgens · 2025-01-15T15:07:03Z

src/Combinatorics/SimplicialComplexes.jl

@@ -670,7 +670,7 @@ function automorphism_group(K::SimplicialComplex; action=:on_vertices)
 end

 @doc raw"""
-    on_simplicial_complex(K::SimplicialComplex, g::PermGroupElem)
+    on_simplicial_complex(K::SimplicialComplex, g::PermGroupElem; action=:on_vertices)


is this maybe a conflict resolution artifact? it does not match the signature of the function this docstring is attached to

antonydellavecchia · 2025-01-15T17:28:52Z

src/Serialization/GAP.jl

+    return G
+  end)
+
+install_GAP_deserialization(


@ThomasBreuer I think I need your help here, and it also makes sense for you to understand what I am doing here anyway.

I am currently try to get the serialization refactor working for gap objects, and the first one that causes an issue is when we try and save subgroups of free groups

If you see above I have a new function called type_params and this handles when params need to be added to the type description for example saving a subgroup of a free group currently looks like this

{ "_ns": { "Oscar": [ "https://github.com/oscar-system/Oscar.jl", "1.3.0-DEV-0324e87a589a2c2c5b6f4b0db4634b14477250ae" ] }, "_refs": { "fef8f069-d2db-4c88-9118-f2b7ba1c3999": { "_type": "GapObj", "data": { "GapType": "IsFreeGroup", "names": [ "f1", "f2" ], "wfilt": "IsSyllableWordsFamily" } } }, "_type": { "name": "GapObj", "params": "fef8f069-d2db-4c88-9118-f2b7ba1c3999" }, "data": { "GapType": "IsFreeGroup", "gens": [ [ "1", "1" ] ] }, "id": "ab56be5f-1bf6-4fec-a3d5-35cb59b6096b" }

Note how there is no longer a freeGroup key in the data section and that the type information of the GapObj now has a params key.

The issue I am having now is that I need to pass the free group down, but this means I need two different function signatures.

But I am guessing these cannot share the same namespace in GAP?

please let me know if you have any questions about some of the changes here

@antonydellavecchia I am not sure whether I understand the aim of the proposed changes.
A subgroup of a full free group is best described in terms of generators relative to the full free group, hence the "old" approach first serializes the full free group and then the generators of the subgroup in question, and the deserialization first reconstructs the full free group and then the subgroup generators w.r.t. the full group.

The idea of the "new" approach seems to be that the function type_params shall describe the object(s) to be serialized together with the data of the given object.
The problem is that type_params for a GapObj has to deal with a lot of different GapObjs.
In the "old" serialization and deserialization of GAP objects, the approach using install_GAP_serialization and install_GAP_deserialization yields small Julia functions for different kinds of GAP objects. One could think of an analogous install_GAP_type_params in order to achieve this for type_params iin the "new" approach, but what is the advantage of using type_params, compared to the "old" (recursive) serialization approach?

Just for curiosity: How does the "new" approach handle objects that need more than one dependent object to be serialized?
(For example, the most natural way to serialize a map seems to be to serialize domain and codomain and information that defines the function in question.)
These dependent objects are stored in _refs, their keys in _refs shall be referenced somewhere in _type, but how does the deserialization code know which is which? In the "old" approach, the references to the keys in _refs are stored in fields inside data (like the freeGroup field), whose meanings are defined by the serialization.

The idea is that type params will do a shallow pass through the object and obtain all necessary type information for the object. This has many benefits, the main one being that this would allow us to put all type information at the top of the file and hence we would be able to read in all params for the object before we get to the data in a linear way.

here is another example of how a the new format looks for a MPolyAnyMap.
You can see that the params section can also have keys, i.e. we can have params that are dictionnaries if necessary
(this does not yet include the linear ordering)

{ "_ns": { "Oscar": [ "https://github.com/oscar-system/Oscar.jl", "1.3.0-DEV-98fc56d6f49a6eacecd525b01c0e543857608f58" ] }, "_refs": { "bede6628-38bf-4803-88df-2ed7487fc8c8": { "_type": { "name": "MPolyRing", "params": { "_type": "QQField" } }, "data": { "symbols": [ "s", "t", "m" ] } }, "fc49a9a8-91af-4b54-888b-58b4f2dd1958": { "_type": { "name": "MPolyRing", "params": { "_type": "QQField" } }, "data": { "symbols": [ "x", "y" ] } } }, "_type": { "name": "MPolyAnyMap", "params": { "codomain": { "name": "MPolyRing", "params": "bede6628-38bf-4803-88df-2ed7487fc8c8" }, "domain": { "name": "MPolyRing", "params": "fc49a9a8-91af-4b54-888b-58b4f2dd1958" } } }, "data": { "images": [ [ [ [ "1", "1", "0" ], "1" ] ], [ [ [ "0", "0", "1" ], "1" ] ] ] } }

Thanks @antonydellavecchia .
Thus the rule is that it is recommended (or mandatory?) to "announce" the dependent objects via type_params.

Then the interface for the (de)serialization for a given object type T (here: GapObj) consists of

a save_object method with second argument T,

a load_object method with two or three arguments, the second argument being T,

a type_params method with one argument of type T, returning S, X where X is either nothing or an object of type S (a dictionary or a GapObj); in the latter case, there must be a three argument method for load_object that takes X as its third argument.

If this is correct then I propose a function install_GAP_type_params as sketched above for the case of GapObj. I can provide the code for that.

The rule will become mandatory.

the load object third parameters will exactly be the second entry of the output from type_params

the output of type_params should be a tuple where the first entry is the type and the second entry are the params for that type, for example above type_params with return the type

PolyAnyMap, Dict(:codomain => QQ[:s, :t, :m], :domain => QQ[:x, :y])

for a group element g It would be some thing like

GroupElem, parent(g)

and yes exactly, if the second output of type_params is nothing, then that particular load_object method only needs two parameters

- introduce `install_GAP_type_params` that implements `type_params` methods based on GAP filters - adjust code for `IsFreeGroup` and `IsSubgroupFpGroup`; now their tests work again

ThomasBreuer · 2025-01-18T20:55:52Z

The tests for pc groups are still disabled. Here the problem is that load_object(s, Vector, (Tuple, [Int, Int, [Vector, Int]]), :comm_rels) does not work anymore.

antonydellavecchia · 2025-01-20T08:55:22Z

The tests for pc groups are still disabled. Here the problem is that load_object(s, Vector, (Tuple, [Int, Int, [Vector, Int]]), :comm_rels) does not work anymore.

Thanks for that. I've made the loading of vectors a little more intuitive.
Please see the changes I've just pushed to get the GAP object serialization working

ThomasBreuer · 2025-01-20T09:00:26Z

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.

lgoettgens · 2025-01-20T14:50:12Z

src/Serialization/LieTheory.jl

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

 @register_serialization_type WeylGroup uses_id

+type_params(W::WeylGroup) = WeylGroup, root_system(W)


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

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

lgoettgens · 2025-01-20T15:20:38Z

src/Serialization/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)...)


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

yes this is probably fine

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

antonydellavecchia added 9 commits September 16, 2024 09:11

some progress on simplifying ref handling in IPC Serialization

45d3516

some progress

3e62a1b

moving things around

8d76aab

type params function

94587e3

some progress on refactoring

e4c5bae

some progress

1dfd391

more progress

c1cbd72

almost completed uni variate polynomials

12eb6ef

uni vairate polynomials serialization passing

84c46e6

antonydellavecchia added the serialization label Sep 28, 2024

antonydellavecchia and others added 10 commits September 29, 2024 16:50

MPolyRing tests passing

e35b665

moving things around

4537cf0

grading needs containers

eb3c734

not sure where i left this

a94c566

new load_type_params

72a96cf

bug with julia 1.11.1

5e06a82

vectors + polynomials working

4cbaee4

some progress on tuples

7255064

reworking save_type_params

b41dee5

tidying

d758fad

antonydellavecchia force-pushed the adv/ipc-serialize branch from 02b2191 to d758fad Compare November 13, 2024 17:16

antonydellavecchia and others added 2 commits November 14, 2024 16:10

dicts working

f61ecd1

containers completed, need some upgrades though

66a5f2e

lkastner previously approved these changes Nov 20, 2024

View reviewed changes

antonydellavecchia added 2 commits November 21, 2024 11:50

tidying

32bb5ee

broke everything

47e3f3f

antonydellavecchia force-pushed the adv/ipc-serialize branch from 47e3f3f to 66a5f2e Compare December 2, 2024 17:14

antonydellavecchia and others added 3 commits December 3, 2024 16:37

fixed containers and polynomials

d971bb5

some issues with field embeddings

beea514

fields tests working now

b6f35d3

antonydellavecchia added 2 commits January 14, 2025 17:50

algebras working

4b37896

started on groups

0b0d1de

Merge branch 'master' into adv/ipc-serialize

62b1bbc

lgoettgens reviewed Jan 15, 2025

View reviewed changes

antonydellavecchia added 2 commits January 15, 2025 17:32

fix, inconsistencies from merge

0324e87

crawling through groups

98fc56d

antonydellavecchia commented Jan 15, 2025

View reviewed changes

antonydellavecchia and others added 4 commits January 16, 2025 12:24

matrices working

49b0768

MPolyMap Working

a26a575

some progress on polyhedral objects

f539b16

adjust GAP serialization to the refactoring

5404f73

- introduce `install_GAP_type_params` that implements `type_params` methods based on GAP filters - adjust code for `IsFreeGroup` and `IsSubgroupFpGroup`; now their tests work again

ThomasBreuer force-pushed the adv/ipc-serialize branch from a5fab69 to 5404f73 Compare January 18, 2025 20:24

GAP object serialization working

0bb0cc7

antonydellavecchia added 3 commits January 20, 2025 10:36

groups working

336c350

removed false kwarg in docs

d322733

Lie Theory serialization working

af036e9

lgoettgens reviewed Jan 20, 2025

View reviewed changes

stuck on regression in polyhedral

215e946

antonydellavecchia force-pushed the adv/ipc-serialize branch from af1675c to 215e946 Compare January 20, 2025 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialization Refactor #4162

Serialization Refactor #4162

antonydellavecchia commented Sep 28, 2024

lgoettgens commented Jan 15, 2025

lgoettgens Jan 15, 2025

antonydellavecchia Jan 15, 2025

ThomasBreuer Jan 16, 2025

antonydellavecchia Jan 16, 2025 •

edited

Loading

ThomasBreuer Jan 16, 2025

antonydellavecchia Jan 16, 2025 •

edited

Loading

ThomasBreuer commented Jan 18, 2025

antonydellavecchia commented Jan 20, 2025

ThomasBreuer commented Jan 20, 2025 •

edited

Loading

lgoettgens Jan 20, 2025

antonydellavecchia Jan 20, 2025

lgoettgens Jan 20, 2025

antonydellavecchia Jan 20, 2025

antonydellavecchia Jan 23, 2025

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

		@register_serialization_type WeylGroup uses_id

		type_params(W::WeylGroup) = WeylGroup, root_system(W)

	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)...)

Serialization Refactor #4162

Are you sure you want to change the base?

Serialization Refactor #4162

Conversation

antonydellavecchia commented Sep 28, 2024

lgoettgens commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonydellavecchia Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonydellavecchia Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

ThomasBreuer commented Jan 18, 2025

antonydellavecchia commented Jan 20, 2025

ThomasBreuer commented Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonydellavecchia Jan 16, 2025 •

edited

Loading

antonydellavecchia Jan 16, 2025 •

edited

Loading

ThomasBreuer commented Jan 20, 2025 •

edited

Loading