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

[FTheoryTools] Overhaul Serialization #3932

Conversation

HereAround
Copy link
Member

@HereAround HereAround commented Jul 12, 2024

  • Use existing dict serialization for FTheory models.
  • Clean up serialization code.
  • Extend serialization of FTheory models to more of their associated data - anything that is currently stored as string, list of strings or boolean values is remembered by the serialization, once this PR is merged.

[I initially hoped to push this so that some of the JSON-files - that currently specify models - can be replaced by their mardi counterparts. This is not possible. Some of the data associated to the models cannot yet be serialized. For instance Lie algebras. I have not checked if there are other data types that we use and lack serialization (for instance graphs, as used for the QSMs. So I postpone this for now.]

cc @antonydellavecchia @apturner @emikelsons

@HereAround HereAround marked this pull request as draft July 12, 2024 12:15
@HereAround HereAround force-pushed the UpdateFTheoryToolsSerialization branch 4 times, most recently from 168338d to 0291fde Compare July 19, 2024 20:01
@HereAround HereAround force-pushed the UpdateFTheoryToolsSerialization branch 3 times, most recently from cb8c1f6 to 9cd6d27 Compare July 19, 2024 20:39
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 98.46154% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.08%. Comparing base (a8d2911) to head (6315982).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3932   +/-   ##
=======================================
  Coverage   84.07%   84.08%           
=======================================
  Files         592      592           
  Lines       81758    81722   -36     
=======================================
- Hits        68739    68713   -26     
+ Misses      13019    13009   -10     
Files Coverage Δ
...eoryTools/src/Serialization/hypersurface_models.jl 100.00% <100.00%> (ø)
experimental/FTheoryTools/src/types.jl 95.00% <100.00%> (+0.88%) ⬆️
...erimental/FTheoryTools/test/hypersurface_models.jl 100.00% <100.00%> (ø)
experimental/FTheoryTools/test/tate_models.jl 100.00% <100.00%> (ø)
...perimental/FTheoryTools/test/weierstrass_models.jl 100.00% <100.00%> (ø)
...ntal/FTheoryTools/src/Serialization/tate_models.jl 97.43% <95.65%> (+5.22%) ⬆️
...heoryTools/src/Serialization/weierstrass_models.jl 97.43% <96.00%> (+5.22%) ⬆️

@HereAround HereAround force-pushed the UpdateFTheoryToolsSerialization branch from 7e31ff8 to 7556ab6 Compare July 20, 2024 12:33
@HereAround HereAround force-pushed the UpdateFTheoryToolsSerialization branch from 7556ab6 to caf4251 Compare July 20, 2024 12:42
@HereAround HereAround marked this pull request as ready for review July 20, 2024 14:34
Copy link
Collaborator

@emikelsons emikelsons left a comment

Choose a reason for hiding this comment

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

Looks good to me and all of the tests passed.

@HereAround
Copy link
Member Author

HereAround commented Jul 20, 2024

Looks good to me and all of the tests passed.

Then I let @antonydellavecchia make the final call. Would like to know if this is ok/acceptable as long as the OSCAR serialization standards are concerned. @antonydellavecchia, if/once you are happy, please consider merging.

@antonydellavecchia antonydellavecchia merged commit 6cf5724 into oscar-system:master Jul 22, 2024
29 checks passed
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.

4 participants