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

first attempt to serialize some groups #2410

Merged
merged 11 commits into from
Nov 11, 2023
Merged

first attempt to serialize some groups #2410

merged 11 commits into from
Nov 11, 2023

Conversation

ThomasBreuer
Copy link
Member

This is just for discussing what we want for groups.

The current code tries to save and restore only data living on the Oscar side, and ignores additional knowledge stored in the objects on the GAP side.
(Still some objects have to be manipulated on the GAP side because they have no counterpart in Oscar; in the current code, this holds for the "full f. p. group" of a subgroup of an f. p. group.)

An alternative approach would be to serialize mainly the objects on the GAP side, together with their known attributes, and to rebuild the Oscar objects from the deserialized GAP objects.
(For serializing/deserializing GAP objects in GAP, there is currently essentially the "pickling/unpickling" approach provided by the IO package.)

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #2410 (182ccdc) into master (a04a050) will increase coverage by 0.12%.
Report is 15 commits behind head on master.
The diff coverage is 94.84%.

@@            Coverage Diff             @@
##           master    #2410      +/-   ##
==========================================
+ Coverage   80.22%   80.35%   +0.12%     
==========================================
  Files         473      483      +10     
  Lines       67257    69365    +2108     
==========================================
+ Hits        53960    55739    +1779     
- Misses      13297    13626     +329     
Files Coverage Δ
src/GAP/gap_to_oscar.jl 96.06% <100.00%> (ø)
src/GAP/wrappers.jl 96.24% <100.00%> (+0.03%) ⬆️
src/Groups/GAPGroups.jl 92.84% <100.00%> (+0.04%) ⬆️
src/Serialization/main.jl 87.05% <100.00%> (-0.83%) ⬇️
src/Serialization/Groups.jl 94.64% <94.64%> (ø)
src/Serialization/GAP.jl 94.33% <94.33%> (ø)

... and 21 files with indirect coverage changes

@ThomasBreuer
Copy link
Member Author

ThomasBreuer commented Jul 19, 2023

The serialization interface promises that one can save some Oscar objects x, y, z, ... to (several) files, such that the following holds.

  1. Loading the contents of the files into the same Oscar session will yield objects that are equal to the given objects x, y, z, ... --this is what test_save_load_roundtrip tests.
  2. If one loads the contents of the files into a different Oscar session then the relations between the loaded objects x', y', z', ... corresponding to x, y, z, ... must be compatible with the relations between the original objects x, y, z, ...; that is, if x and y are elements in the same group then also x' and y' must be elements in the same group.

For an object that is based on a GAP object, requirement 1. makes it necessary to record the information about the FamilyObj value of the underlying GAP object, because otherwise we cannot create an object in the correct family.
The idea of the current code is to keep a dictionary of families of objects that have been serialized in the current Oscar session, and to make the keys for the families part of the serialization information.
Concerning requirement 2., dictionaries of families of deserialized objects from other Oscar sessions are kept, in order to create compatible objects. (Testing that deserializing objects from earlier Oscar sessions works seems to require that some serialization output becomes part of the test input; this part of the input may then need updates when the file format changes.)

It is likely that using Julia's objectid for computing unique keys of GAP family objects is not safe; perhaps it will be necessary to provide a function on the GAP side that yields a unique key.

(Note that GAP's "pickling/unpickling" approach does not deal (yet) with objects which rely on the identity of families.)

@ThomasBreuer
Copy link
Member Author

ThomasBreuer commented Jul 20, 2023

Currently two tests are failing:
Run tests / test (1.6, ubuntu-latest) (pull_request) was canceled, and Run tests / test (1.9, ubuntu-latest) found an error at test/AlgebraicGeometry/Schemes/duValSing.jl:16, which is not likely to be related to the changes from this pull request.

(O.k., restarted the tests, now they passed.)

@antonydellavecchia
Copy link
Collaborator

you can take a look at my branch to see how one would go about reformatting for the refactor
https://github.com/oscar-system/Oscar.jl/tree/adv/start-reformat-serialize-groups

src/Serialization/GAP.jl Outdated Show resolved Hide resolved
src/Serialization/GAP.jl Outdated Show resolved Hide resolved
src/Serialization/GAP.jl Outdated Show resolved Hide resolved
src/Serialization/GAP.jl Outdated Show resolved Hide resolved
src/Serialization/GAP.jl Outdated Show resolved Hide resolved
src/Serialization/GAP.jl Outdated Show resolved Hide resolved
@ThomasBreuer ThomasBreuer marked this pull request as ready for review November 3, 2023 14:13
@antonydellavecchia
Copy link
Collaborator

After a quick look, seems good for a first attempt

@thofma thofma closed this Nov 10, 2023
@thofma thofma reopened this Nov 10, 2023
@thofma
Copy link
Collaborator

thofma commented Nov 10, 2023

I restarted CI and merge once it is green. I would really like to use this.

@thofma thofma merged commit 90e093f into oscar-system:master Nov 11, 2023
32 of 42 checks passed
@thofma
Copy link
Collaborator

thofma commented Nov 11, 2023

Thanks @ThomasBreuer!

@ThomasBreuer ThomasBreuer deleted the TB_serialize_groups branch November 11, 2023 08:55
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.

None yet

4 participants