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

Make PeaksAndMetrics pickle-able #1195

Merged
merged 14 commits into from Mar 22, 2017

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Mar 20, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 20, 2017

Coverage Status

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2017

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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2017

@@ -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)

This comment has been minimized.

@matthew-brett

matthew-brett Mar 20, 2017

Member

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2017

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2017

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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 20, 2017

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

This comment has been minimized.

codecov-io commented Mar 20, 2017

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

This comment has been minimized.

Member

arokem commented Mar 20, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2017

arokem added some commits Mar 20, 2017

Documentation and a bit more testing.
Checking that switching on and off some of the requested returns still de-/serializes properly.
@arokem

This comment has been minimized.

Member

arokem commented Mar 20, 2017

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

This comment has been minimized.

Member

arokem commented Mar 20, 2017

Changing the title of the PR accordingly 😉

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

@coveralls

This comment has been minimized.

coveralls commented Mar 20, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

coveralls commented Mar 20, 2017

Coverage Status

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

@@ -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,

This comment has been minimized.

@matthew-brett

matthew-brett Mar 21, 2017

Member

Why not make this a classmethod called from_params or similar?

This comment has been minimized.

@arokem

arokem Mar 21, 2017

Member

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

This comment has been minimized.

Member

arokem commented Mar 21, 2017

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

This comment has been minimized.

Member

arokem commented Mar 21, 2017

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

This comment has been minimized.

Member

matthew-brett commented Mar 21, 2017

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

This comment has been minimized.

Member

arokem commented Mar 21, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2017

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

Any ideas?

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

This comment has been minimized.

@matthew-brett

matthew-brett Mar 22, 2017

Member

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

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

This comment has been minimized.

@matthew-brett

matthew-brett Mar 22, 2017

Member

No longer optional !

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 22, 2017

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

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

This comment has been minimized.

@matthew-brett

matthew-brett Mar 22, 2017

Member

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

This comment has been minimized.

Member

arokem commented Mar 22, 2017

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 22, 2017

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

This comment has been minimized.

Member

arokem commented Mar 22, 2017

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 22, 2017

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

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

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

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

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

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
coverage/coveralls Coverage increased (+0.005%) to 88.435%
Details
@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

Beat you to it!

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 22, 2017

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

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

@arokem

This comment has been minimized.

Member

arokem commented Mar 22, 2017

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Mar 22, 2017

Well played sir.

@coveralls

This comment has been minimized.

coveralls commented Mar 22, 2017

Coverage Status

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

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

Merge pull request nipy#1195 from arokem/pickle-pam-3.6
Make PeaksAndMetrics pickle-able
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment