Make PeaksAndMetrics pickle-able #1195

arokem · 2017-03-20T15:51:09Z

Might help with #1190

See: http://stackoverflow.com/a/2050357/3532933

coveralls · 2017-03-20T17:12:26Z

Coverage remained the same at 88.431% when pulling 7d53841 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

arokem · 2017-03-20T17:12:38Z

Travis failure unrelated to this PR, and only on Python 3.3

matthew-brett · 2017-03-20T18:12:37Z

Further explanation : https://snorfalorpagus.net/blog/2016/04/16/pickling-cython-classes

matthew-brett · 2017-03-20T18:14:00Z

dipy/reconst/peak_direction_getter.pyx

@@ -38,6 +38,10 @@ cdef class PeaksAndMetricsDirectionGetter(DirectionGetter):
        self.ang_thr = 60
        self.total_weight = .5

+    def __getstate__(self): return self.__dict__
+
+    def __setstate__(self, d): self.__dict__.update(d)


The update will leave any previous members in place, that are not defined in d. Is that the right thing to do?

arokem · 2017-03-20T18:16:51Z

Thanks for finding that post! Do you understand why we don't see this in other versions of Python? Seems like we should have been getting this as far back as at least 3.4.

…

On Mon, Mar 20, 2017 at 11:14 AM, Matthew Brett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/reconst/peak_direction_getter.pyx <#1195 (comment)>: > @@ -38,6 +38,10 @@ cdef class PeaksAndMetricsDirectionGetter(DirectionGetter): self.ang_thr = 60 self.total_weight = .5 + def __getstate__(self): return self.__dict__ + + def __setstate__(self, d): self.__dict__.update(d) The update will leave any previous members in place, that are not defined in d. Is that the right thing to do? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNiTzfk2CVuZN6pwgXO-nTSLPkpVDks5rnsHogaJpZM4Min2V> .

matthew-brett · 2017-03-20T18:18:36Z

No, that is puzzling. I wonder whether it's something random about when classes get thrown around by the multiprocessing ?

matthew-brett · 2017-03-20T18:19:43Z

Sorry - I hate to say - but it would be good to have a test of pickling and unpickling these guys in fresh and used state.

codecov-io · 2017-03-20T18:21:21Z

Codecov Report

Merging #1195 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
+ Coverage   85.93%   85.94%   +<.01%     
==========================================
  Files         219      219              
  Lines       26403    26419      +16     
  Branches     2711     2716       +5     
==========================================
+ Hits        22689    22705      +16     
+ Misses       3052     3050       -2     
- Partials      662      664       +2

Impacted Files	Coverage Δ
dipy/direction/peaks.py	`79.35% <100%> (+0.19%)`	⬆️
dipy/direction/tests/test_peaks.py	`99.44% <100%> (+0.02%)`	⬆️
dipy/utils/six.py	`45.71% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab2032b...02c4ee7. Read the comment docs.

arokem · 2017-03-20T18:21:27Z

To answer the other thing: From reading the documentation, it seems that maybe we don't even need to implement `__setstate__`, so I have removed that one.

…

On Mon, Mar 20, 2017 at 11:16 AM, Ariel Rokem ***@***.***> wrote: Thanks for finding that post! Do you understand why we don't see this in other versions of Python? Seems like we should have been getting this as far back as at least 3.4. On Mon, Mar 20, 2017 at 11:14 AM, Matthew Brett ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In dipy/reconst/peak_direction_getter.pyx > <#1195 (comment)>: > > > @@ -38,6 +38,10 @@ cdef class PeaksAndMetricsDirectionGetter(DirectionGetter): > self.ang_thr = 60 > self.total_weight = .5 > > + def __getstate__(self): return self.__dict__ > + > + def __setstate__(self, d): self.__dict__.update(d) > > The update will leave any previous members in place, that are not > defined in d. Is that the right thing to do? > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1195 (review)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAHPNiTzfk2CVuZN6pwgXO-nTSLPkpVDks5rnsHogaJpZM4Min2V> > . >

arokem · 2017-03-20T18:22:06Z

Yes. Fair enough. Since I just did that in a notebook, to be sure that I am not messing anything up, may as well add this to the tests... Coming!

…

On Mon, Mar 20, 2017 at 11:19 AM, Matthew Brett ***@***.***> wrote: Sorry - I hate to say - but it would be good to have a test of pickling and unpickling these guys in fresh and used state. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNgTCBaO6RelQ6EBV8_XtOKXgvz1Tks5rnsNAgaJpZM4Min2V> .

arokem · 2017-03-20T18:42:02Z

OK - here it is. Let's see if I managed to write something that works on both Python 2 and Python 3...

…

On Mon, Mar 20, 2017 at 11:21 AM, Ariel Rokem ***@***.***> wrote: Yes. Fair enough. Since I just did that in a notebook, to be sure that I am not messing anything up, may as well add this to the tests... Coming! On Mon, Mar 20, 2017 at 11:19 AM, Matthew Brett ***@***.***> wrote: > Sorry - I hate to say - but it would be good to have a test of pickling > and unpickling these guys in fresh and used state. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#1195 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAHPNgTCBaO6RelQ6EBV8_XtOKXgvz1Tks5rnsNAgaJpZM4Min2V> > . >

Following roughly this idea: http://stackoverflow.com/a/12712509/3532933

Checking that switching on and off some of the requested returns still de-/serializes properly.

arokem · 2017-03-20T21:47:46Z

I'm learning so many things about pickles over here 😄

Previous attempt seemed to be failing on Python 2. Here's a new approach that might work on both.

arokem · 2017-03-20T21:48:20Z

Changing the title of the PR accordingly 😉

coveralls · 2017-03-20T22:11:04Z

Coverage increased (+0.01%) to 88.442% when pulling d4bed65 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

coveralls · 2017-03-20T22:46:31Z

Coverage increased (+0.01%) to 88.442% when pulling d4bed65 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

matthew-brett · 2017-03-21T08:12:37Z

dipy/direction/peaks.py

@@ -157,8 +157,63 @@ def peak_directions(odf, sphere, relative_peak_threshold=.5,
    return directions, values, indices


+def _rebuild_pam(sphere, peak_indices, peak_values, peak_dirs,


Why not make this a classmethod called from_params or similar?

It would be a bit weird as a class method, considering that it returns another instance of that class. Or would it?

There does seem to be an opportunity to reduce boiler-plate here though, so I am going to try out a couple more things. Stand by.

arokem · 2017-03-21T14:46:20Z

What do you think about this? I renamed it to _pam_from_attrs and I am now also using it to initialize the PAM instance in the peaks_from_model function below (which had a similar pattern). I prefer attrs to params, to not confuse this with model parameters.

arokem · 2017-03-21T14:47:31Z

And I think this looks better than:

pam = PeaksAndMetrics()
pam = pam._from_attrs(...)

which is what we would do if we had this implemented as a class method (I think).

matthew-brett · 2017-03-21T16:27:50Z

The floating external function looks funny to me, especially for a class that is otherwise empty.

Here's the alternative classmethod I was thinking of:

pam = PeaksAndMetrics()
pam = PeaksAndMetrics.from_attrs(...)

To me it looks like a normal alternative class constructor pattern...

arokem · 2017-03-21T17:02:24Z

Gotcha. How's this?

…

On Tue, Mar 21, 2017 at 9:27 AM, Matthew Brett ***@***.***> wrote: The floating external function looks funny to me, especially for a class that is otherwise empty. Here's the alternative classmethod I was thinking of: pam = PeaksAndMetrics() pam = PeaksAndMetrics.from_attrs(...) To me it looks like a normal alternative class constructor pattern... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNqqB5cFWz7rMV4ovzdv-zF6e-t5wks5rn_qGgaJpZM4Min2V> .

arokem · 2017-03-21T18:01:22Z

Sadly, this approach does not work on Python 2: https://travis-ci.org/arokem/dipy/jobs/213491090#L2580

Any ideas?

arokem · 2017-03-22T19:13:45Z

On Wed, Mar 22, 2017 at 11:50 AM, Matthew Brett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/direction/peaks.py <#1195 (comment)>: > @@ -157,8 +157,64 @@ def peak_directions(odf, sphere, relative_peak_threshold=.5, return directions, values, indices +def _pam_from_attrs(sphere, peak_indices, peak_values, peak_dirs, Consider adding class as first argument? Or as optional argument at end? In case we need to sub-class? Sorry to ask, but why do we need this otherwise empty sub-class? class PeaksAndMetrics(PeaksAndMetricsDirectionGetter): pass Why not use PeaksAndMetricsDirectionGetter directly?

I'm not sure why we are subclassing here. I added another argument for sub-classing purposes, with PeaksAndMetrics as the default value.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNstK6GtBOdQXyPzghArKUlCD1MhGks5roW1rgaJpZM4Min2V> .

matthew-brett · 2017-03-22T19:32:29Z

dipy/direction/peaks.py

+                                                   self.qa,
+                                                   self.shm_coeff,
+                                                   self.B,
+                                                   self.odf)


Add self.__class__ as argument to allow sub-classing? Maybe, it would be better as first argument.

arokem · 2017-03-22T19:40:29Z

Yup. Moved to first argument.

…

On Wed, Mar 22, 2017 at 12:32 PM, Matthew Brett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/direction/peaks.py <#1195 (comment)>: > @@ -157,8 +157,20 @@ def peak_directions(odf, sphere, relative_peak_threshold=.5, return directions, values, indices +class PeaksAndMetrics(PeaksAndMetricsDirectionGetter): + def __reduce__(self): return _pam_from_attrs, (self.sphere, + self.peak_indices, + self.peak_values, + self.peak_dirs, + self.gfa, + self.qa, + self.shm_coeff, + self.B, + self.odf) Add self.__class__ as argument to allow sub-classing? Maybe, it would be better as first argument. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNvBDV_x6hWndgXUBl7Pm_dehf4Qtks5roXdNgaJpZM4Min2V> .

matthew-brett · 2017-03-22T20:12:22Z

dipy/direction/peaks.py

+
+    Parameters
+    ----------
+    klass : class, optional


No longer optional !

coveralls · 2017-03-22T20:13:21Z

Coverage increased (+0.007%) to 88.438% when pulling 8f221a0 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

matthew-brett · 2017-03-22T20:19:13Z

All OK for me, after fix of optional in docstring.

arokem · 2017-03-22T20:21:34Z

Good catch. Thanks!

…

On Wed, Mar 22, 2017 at 1:19 PM, Matthew Brett ***@***.***> wrote: All OK for me, after fix of optional in docstring. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNohi8DTN73w_4yPYMEPmbaD1dRuOks5roYJCgaJpZM4Min2V> .

coveralls · 2017-03-22T21:11:44Z

Coverage increased (+0.007%) to 88.438% when pulling f47076a on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

matthew-brett · 2017-03-22T21:19:04Z

dipy/direction/peaks.py


-    return pam
+    return _pam_from_attrs(PeaksAndMetrics, sphere, peak_indices, peak_values,


I guess, if you can be bothered:

return _pam_from_attrs(PeaksAndMetrics, sphere, peak_indices, peak_values, peak_dirs, gfa_array, qa_array, shm_coeff if return_sh else None, B if return_sh else None, odf_array if return_odf else None)

arokem · 2017-03-22T21:36:01Z

I can always be bothered

…

On Wed, Mar 22, 2017 at 2:19 PM, Matthew Brett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In dipy/direction/peaks.py <#1195 (comment)>: > - return pam + return _pam_from_attrs(PeaksAndMetrics, sphere, peak_indices, peak_values, I guess, if you can be bothered: return _pam_from_attrs(PeaksAndMetrics, sphere, peak_indices, peak_values, peak_dirs, gfa_array, qa_array, shm_coeff if return_sh else None, B if return_sh else None, odf_array if return_odf else None) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNm74w010ktnrBIrBdBSYY4IfFJi8ks5roZBIgaJpZM4Min2V> .

matthew-brett · 2017-03-22T21:37:22Z

Ah sorry - I mean particularly cleaning up the slightly ugly if blocks with the conditionals:

...
shm_coeff if return_sh else None,
B if return_sh else None,
odf_array if return_odf else None)

arokem · 2017-03-22T21:45:23Z

Oops. I didn't really bother reading all the way down :-) Fixed now.

…

On Wed, Mar 22, 2017 at 2:37 PM, Matthew Brett ***@***.***> wrote: Ah sorry - I mean particularly cleaning up the slightly ugly if blocks with the conditionals: ... shm_coeff if return_sh else None, B if return_sh else None, odf_array if return_odf else None) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNlIKWOUo6AabbIWW-bH672K2YfvWks5roZSTgaJpZM4Min2V> .

matthew-brett · 2017-03-22T21:46:48Z

Thanks for your patience. One of us merge when the tests pass (apart from the 3.3 failure)?

coveralls · 2017-03-22T21:53:40Z

Coverage increased (+0.007%) to 88.438% when pulling ec3ac55 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

arokem · 2017-03-22T21:56:24Z

The patience is all yours. Last one to the merge button is a rotten egg.

coveralls · 2017-03-22T22:37:59Z

Coverage increased (+0.005%) to 88.435% when pulling 02c4ee7 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

coveralls · 2017-03-22T23:37:00Z

Coverage increased (+0.005%) to 88.435% when pulling 02c4ee7 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

arokem · 2017-03-22T23:43:40Z

Beat you to it!

matthew-brett · 2017-03-22T23:47:32Z

Good job - but - aren't the tests still running?

arokem · 2017-03-22T23:49:44Z

I think not: https://travis-ci.org/arokem/dipy/builds/214021129

…

On Wed, Mar 22, 2017 at 4:47 PM, Matthew Brett ***@***.***> wrote: Good job - but - aren't the tests still running? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1195 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNu7BfJeAVc72f7C-3IeH-tRlQNtyks5robMVgaJpZM4Min2V> .

arokem · 2017-03-22T23:50:34Z

Addmitedly, I cheated a little bit, because I have Travis looking over my own fork as well :-)

…

On Wed, Mar 22, 2017 at 4:49 PM, Ariel Rokem ***@***.***> wrote: I think not: https://travis-ci.org/arokem/dipy/builds/214021129 On Wed, Mar 22, 2017 at 4:47 PM, Matthew Brett ***@***.***> wrote: > Good job - but - aren't the tests still running? > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#1195 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AAHPNu7BfJeAVc72f7C-3IeH-tRlQNtyks5robMVgaJpZM4Min2V> > . >

matthew-brett · 2017-03-22T23:51:08Z

Well played sir.

coveralls · 2017-03-22T23:58:52Z

Coverage increased (+0.005%) to 88.435% when pulling 02c4ee7 on arokem:pickle-pam-3.6 into ab2032b on nipy:master.

Make PeaksAndMetrics pickle-able

Add get_state, set_state methods to PeaksAndMetrics.

7d53841

arokem mentioned this pull request Mar 20, 2017

Pickle error for Python 3.6 and test_peaksFromModelParallel #1190

Closed

matthew-brett reviewed Mar 20, 2017

View reviewed changes

Don't implement set_state.

399ac2e

Test pickling of PAM objects.

2de6ca7

arokem added 2 commits March 20, 2017 14:06

Implement pickling of PAM objects as a reduce operation instead.

bcbdeac

Following roughly this idea: http://stackoverflow.com/a/12712509/3532933

Documentation and a bit more testing.

d4bed65

Checking that switching on and off some of the requested returns still de-/serializes properly.

arokem changed the title ~~Add get_state, set_state methods to PeaksAndMetrics.~~ Make PeaksAndMetrics pickle-able Mar 20, 2017

matthew-brett reviewed Mar 21, 2017

View reviewed changes

Rename to _pam_from_attrs, use to reduce boilerplate.

9743fdb

Implement from_attrs as an alternative constructor classmethod.

cec60bc

matthew-brett reviewed Mar 22, 2017

View reviewed changes

Make the klass input obligatory.

a2d49bc

Whitespace.

8f221a0

matthew-brett reviewed Mar 22, 2017

View reviewed changes

dipy/direction/peaks.py Outdated

Parameters

----------

klass : class, optional

Copy link

Contributor

matthew-brett Mar 22, 2017

Choose a reason for hiding this comment

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

No longer optional !

Correct docstring: this is not optional!

f47076a

matthew-brett reviewed Mar 22, 2017

View reviewed changes

Whitespace.

ec3ac55

Prettify the if statements.

02c4ee7

arokem merged commit 0ea8af7 into dipy:master Mar 22, 2017

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request dipy#1195 from arokem/pickle-pam-3.6

d23f8f1

Make PeaksAndMetrics pickle-able

		@@ -157,8 +157,63 @@ def peak_directions(odf, sphere, relative_peak_threshold=.5,
		return directions, values, indices


		def _rebuild_pam(sphere, peak_indices, peak_values, peak_dirs,


		return pam
		return _pam_from_attrs(PeaksAndMetrics, sphere, peak_indices, peak_values,

Make PeaksAndMetrics pickle-able #1195

Make PeaksAndMetrics pickle-able #1195

Conversation

arokem commented Mar 20, 2017

coveralls commented Mar 20, 2017

arokem commented Mar 20, 2017

matthew-brett commented Mar 20, 2017

matthew-brett Mar 20, 2017

Choose a reason for hiding this comment

arokem commented Mar 20, 2017 via email

matthew-brett commented Mar 20, 2017

matthew-brett commented Mar 20, 2017

codecov-io commented Mar 20, 2017 • edited

Codecov Report

arokem commented Mar 20, 2017 via email

arokem commented Mar 20, 2017 via email

arokem commented Mar 20, 2017 via email

arokem commented Mar 20, 2017

arokem commented Mar 20, 2017

coveralls commented Mar 20, 2017

coveralls commented Mar 20, 2017

matthew-brett Mar 21, 2017

Choose a reason for hiding this comment

arokem Mar 21, 2017

Choose a reason for hiding this comment

arokem commented Mar 21, 2017

arokem commented Mar 21, 2017 • edited

matthew-brett commented Mar 21, 2017

arokem commented Mar 21, 2017 via email

arokem commented Mar 21, 2017

arokem commented Mar 22, 2017 via email

matthew-brett Mar 22, 2017

Choose a reason for hiding this comment

arokem commented Mar 22, 2017 via email

matthew-brett Mar 22, 2017

Choose a reason for hiding this comment

coveralls commented Mar 22, 2017

matthew-brett commented Mar 22, 2017

arokem commented Mar 22, 2017 via email

coveralls commented Mar 22, 2017

matthew-brett Mar 22, 2017

Choose a reason for hiding this comment

arokem commented Mar 22, 2017 via email

matthew-brett commented Mar 22, 2017

arokem commented Mar 22, 2017 via email

matthew-brett commented Mar 22, 2017

coveralls commented Mar 22, 2017

arokem commented Mar 22, 2017

coveralls commented Mar 22, 2017

coveralls commented Mar 22, 2017

arokem commented Mar 22, 2017

matthew-brett commented Mar 22, 2017

arokem commented Mar 22, 2017 via email

arokem commented Mar 22, 2017 via email

matthew-brett commented Mar 22, 2017

coveralls commented Mar 22, 2017

codecov-io commented Mar 20, 2017 •

edited

arokem commented Mar 21, 2017 •

edited