-
Notifications
You must be signed in to change notification settings - Fork 47
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
Use cloudpickle for serializing NegLogParameterPriors #1467
Conversation
Cloudpickle is able to handle more complex objects than pickle. See ICB-DCM#1465 Closes ICB-DCM#1465 If the new process are forked, we could skip the pickling, but at this point, I don't think that's necessary.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1467 +/- ##
===========================================
- Coverage 82.87% 82.83% -0.05%
===========================================
Files 163 163
Lines 13760 13765 +5
===========================================
- Hits 11404 11402 -2
- Misses 2356 2363 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to perhaps test with specifically a model where it previously failed (e.g. Schwen_PONE2014 from #1465)?
Generally, yes. I already have that lying around. But it will be another slow test due to model import, that's why I left it out so far. |
As we always have the problem to balance between "elaborate" test vs time. Would it make sense to have a set of tests that only run in the (I think weekly) master test? |
In principle, yes, but this should not become an excuse for not writing more efficient tests. |
Added a simpler test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :) If cloudpickle is more powerful then pickle, should we switch to cloudpickle everywhere?
It seems pickle got the job done in most cases so far. I'd only do it where necessary. |
Cloudpickle is able to handle more complex objects than pickle.
See #1465
Closes #1465