Serialize Types with Attributes #4020
Conversation
…ticModel and GroupBasedPhylogeneticModel
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4020 +/- ##
==========================================
+ Coverage 84.55% 84.58% +0.03%
==========================================
Files 597 597
Lines 82180 82265 +85
==========================================
+ Hits 69490 69587 +97
+ Misses 12690 12678 -12
|
Co-authored-by: Lars Göttgens <[email protected]>
|
I'll try to get this branch now working with the Lie algebras to see if that is indeed what I imagined it to be |
Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
Co-authored-by: Lars Göttgens <[email protected]>
src/Serialization/main.jl
Outdated
| 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) | ||
| end | ||
| end |
There was a problem hiding this comment.
this, currently, always saves a dict, which may often be empty. In particular, this also gets saved when passing with_attrs=false. To circumvent this, you could add something like the following:
| 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) | |
| end | |
| end | |
| if any(attr -> has_attribute(obj, attr), attrs_list(s, T)) | |
| 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) | |
| end | |
| end | |
| end |
There was a problem hiding this comment.
ill add the check here to not block lie algebra serialization, but also keep the check in toric varieties for now, and see if other similar examples popup and adjust them all at the same time.
src/Serialization/ToricGeometry.jl
Outdated
| function save_object(s::SerializerState, ntv::T) where T <: NormalToricVarietyType | ||
| attrs = attrs_list(s, T) | ||
|
|
||
| if !isnothing(attrs) && any([has_attribute(ntv, attr) for attr in attrs]) |
There was a problem hiding this comment.
attrs will never be nothing, I think you need isempty for what you had here earlier.
you could, in theory, just save NTV from now on always as a dicts with pm_data and attrs, and only handle the old format in loading. this would make the saving already simpler, and it would not double the check for the presence of any attribute with https://github.com/oscar-system/Oscar.jl/pull/4020/files#r1723508786
There was a problem hiding this comment.
yes, I think the check here instead makes sense, this would then altert he toric variety files and this will introduce a breaking change, i'll need to write an upgrade script + probably upgrade martin's artifacts for f theory.
There was a problem hiding this comment.
cf #4020 (comment)
tldr: I am not involved in NTV, so feel free to ignore my comments about that and instead talk to the people having a say there :)
|
@antonydellavecchia are you happy with having this merged once CI succeeds? |
I would @benlorenz to have a quick look before |
src/Serialization/main.jl
Outdated
| save(io::IO, obj::Any; metadata::MetaData=nothing) | ||
| save(filename::String, obj::Any, metadata::MetaData=nothing) | ||
| save(io::IO, obj::Any; metadata::MetaData=nothing, with_attrs::Bool=false) | ||
| save(filename::String, obj::Any, metadata::MetaData=nothing, with_attrs::Bool=false) |
There was a problem hiding this comment.
This now longer matches the changed default
There was a problem hiding this comment.
thanks!
* first attempts at writing the serializers for the new types PhylogeneticModel and GroupBasedPhylogeneticModel * include serialization functions from AlgebraicStatistics * first steps to serializing attributes * saving and loading attributes working * added some documentation on with_attrs * updated docs, + removed algebraic stats commits, and fixed comment * Update experimental/AlgebraicStatistics/src/AlgebraicStatistics.jl * Apply suggestions from code review Co-authored-by: Lars Göttgens <[email protected]> * includes load_attrs and save_attrs in import all function * Apply suggestions from code review Co-authored-by: Lars Göttgens <[email protected]> * changed type_attr_map to a function * type_attr_map -> type_attr_dict * uses encode type to setup type_attr_map * still issues with the test * More `Type` -> `String` * forgot to with_attrs to save in setup_tests * toric tests passing * moved save_attrs to appropriate line * moved attrs_list to appropriate line * Update src/Serialization/main.jl Co-authored-by: Lars Göttgens <[email protected]> * Update src/Serialization/main.jl Co-authored-by: Lars Göttgens <[email protected]> * Update src/Serialization/serializers.jl Co-authored-by: Lars Göttgens <[email protected]> * Update src/Serialization/main.jl Co-authored-by: Lars Göttgens <[email protected]> * small fix still need to move the check * added check inside save_attrs * offline review * fixed save docs --------- Co-authored-by: Marius <[email protected]> Co-authored-by: Lars Göttgens <[email protected]>
A first pull request on serialization of attributes. Adds the keyword argument
with_attrs.Includes updates to register_serialization_type that allows developers to add a vector of symbols
referring to attributes they would like to be serialized with the given type.
This is a first step for the serialization involved in #4008
and #3980