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 attempts at writing the serializers for the new types PhylogeneticModel and GroupBasedPhylogeneticModel #4010

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

Conversation

mariusneu
Copy link
Contributor

I am currently trying to serialize the new data types introduced in the algebraic statistics (sub)project on algebraic phylogenetics by @marinagarrote et al (cf. #3812). Following the documentation and writing the code as naive as possible, I got problems when testing the functions by trying to save an instance of GroupPhylogeneticModel, with the following error message: Unsupported type 'GroupBasedPhylogeneticModel' for encoding.

Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.73%. Comparing base (6f856d1) to head (dd43585).
Report is 132 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4010      +/-   ##
==========================================
+ Coverage   84.58%   84.73%   +0.14%     
==========================================
  Files         596      630      +34     
  Lines       81969    84510    +2541     
==========================================
+ Hits        69336    71606    +2270     
- Misses      12633    12904     +271     
Files with missing lines Coverage Δ
...ntal/AlgebraicStatistics/src/PhylogeneticModels.jl 100.00% <100.00%> (ø)
...erimental/AlgebraicStatistics/src/serialization.jl 100.00% <100.00%> (ø)
experimental/AlgebraicStatistics/test/runtests.jl 100.00% <100.00%> (ø)
src/Serialization/Combinatorics.jl 88.37% <100.00%> (+1.88%) ⬆️
src/Serialization/containers.jl 94.39% <100.00%> (+6.64%) ⬆️

... and 188 files with indirect coverage changes

save_object(s, graph(pm), :tree) # already implemented
save_object(s, number_states(pm), :states) # already implemented
save_object(s, probability_ring(pm), :probability_ring) # already implemented
save_object(s, root_distribution(pm), :root_distribution) # needs to be implemented, or we change the entry type to QQFieldElem
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can using save_typed_object here if the type isn't always know, or if your using a vector any because not all entries are the same type you'll need to use a tuple

@@ -19,6 +20,19 @@ function load_object(s::DeserializerState, g::Type{Graph{T}}) where T <: Union{D
return g(smallobj)
end

function save_object(s::SerializerState, e::Edge)
save_data_dict(s) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can be done using save_data_array instead, it'll make file sizes smaller

antonydellavecchia and others added 2 commits August 21, 2024 17:58
…zation of dicts to catch the case where the key is not serialized with params


function load_object(s::DeserializerState, g::Type{PhylogeneticModel})
graph = load_object(s, Graph{Directed}, :tree)
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you would need to use

Suggested change
graph = load_object(s, Graph{Directed}, :tree)
graph = load_typed_object(s, :tree)

@mariusneu
Copy link
Contributor Author

mariusneu commented Sep 27, 2024

In commit 8723fc9, tests did not pass so I investigated it and found out that apparently object identity does not the same with polymake-derived objects? Therefore, the graph objects of the original model and the loaded one were not identical, although they were the same tree. I wrote a workaround using adjacency_matrix(graph)

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.

2 participants