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

Enable pickling and copying #1

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Enable pickling and copying #1

wants to merge 10 commits into from

Conversation

fabmazz
Copy link
Member

@fabmazz fabmazz commented Sep 14, 2022

Enables pickling of sib.Test, and copying of sib.Params and sib.FactorGraph. This is needed in some cases when the rankers need to be copied in-memory.

Also, align the code in some places, and add some comments.

@fabmazz fabmazz requested a review from abraunst September 14, 2022 14:57
@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Merging #1 (d15eeec) into master (8a6e76d) will increase coverage by 0.65%.
The diff coverage is 97.15%.

❗ Current head d15eeec differs from pull request most recent head 73d5dba. Consider uploading reports for the commit 73d5dba to get more accurate results

@@            Coverage Diff             @@
##           master       #1      +/-   ##
==========================================
+ Coverage   62.09%   62.73%   +0.65%     
==========================================
  Files           8        8              
  Lines         910      931      +21     
==========================================
+ Hits          565      584      +19     
- Misses        345      347       +2     
Impacted Files Coverage Δ
pysib.cpp 66.67% <95.24%> (-1.19%) ⬇️
bp.cpp 75.18% <100.00%> (ø)
bp.h 100.00% <100.00%> (+7.70%) ⬆️

... and 1 file with indirect coverage changes

.def("__repr__", &lexical_cast<string, Params>);
.def("__repr__", &lexical_cast<string, Params>)
.def("__copy__", [](const Params &s){
return Params(s);
Copy link
Member

Choose a reason for hiding this comment

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

Params contains smart pointers... are we sure that this is the correct way to do this? In the sense that this will be not be an "honest" deep copy, as e.g. prob_i and prob_r will still point to the old python object I think...

return Params(s);
})
.def("__deepcopy__", [](const Params &s, py::dict){
return Params(s);
Copy link
Member

Choose a reason for hiding this comment

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

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants