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

Improve performance of tissue classification #2174

Merged
merged 4 commits into from Jul 23, 2020

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented May 19, 2020

This PR improves the performance of tissue classification by modifying the Cython code in the seg_stats method of ConstantObservationModel to avoid high overhead. There should be no change in behavior with this PR.

This initial call to seg_stats in the classify method of TissueClassifierHMRF for the example data in doc/demo/tissue_classification.py takes > 60 seconds on my system prior to this PR and ~60 ms afterwards. So, the setup prior to iteration is now nearly instantaneous. The iterated conditional modes loop itself still takes the same time as before (~2 minutes), giving an overall speedup of around 30% for the demo.

I would recommend reviewing the first commit in isolation as it has all of the important changes.
The remaining commits are just minor maintenance/style fixes.

@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #2174 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2174   +/-   ##
=======================================
  Coverage   91.37%   91.37%           
=======================================
  Files         251      251           
  Lines       32449    32449           
  Branches     3408     3408           
=======================================
  Hits        29651    29651           
  Misses       2057     2057           
  Partials      741      741           

@@ -217,48 +239,7 @@ class ConstantObservationModel(object):
return mu_upd, var_upd


def update_param_new(self, image, P_L_Y, mu, nclasses):
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this was not used anywhere @grlee77 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it seems to be some leftover dead code. I did a quick grep of the code base just to be sure and don't see any other files that refer to it.

cnp.npy_intp [::1] num_vox_view

# ravel C-contiguous array so we can use 1D indexing in the loop below
input_view = np.ascontiguousarray(input_image, dtype=np.double).ravel()
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so here is the trick that speeds up the code. Have a look @villalonreina

@Garyfallidis
Copy link
Contributor

+1 on my side. AFAIK this is ready to be merged. @villalonreina do have a look. If @villalonreina does not get back to us during the next 2 days then we can move ahead. This loos pretty safe. We still may need to wait for the CLI issues. Thank you @grlee77.

@skoudoro
Copy link
Member

Hi @grlee77,

CI's are back to green ! So can you just rebase this PR to make sure that everything is ok? and then, the code can be merged.

Thank you!

@skoudoro skoudoro added this to the 1.2 milestone Jul 22, 2020
@grlee77
Copy link
Contributor Author

grlee77 commented Jul 23, 2020

Hi @skoudoro, I rebased this on master. The two Azure failures seem like unrelated errors due to a network error/timeout when downloading packages. Can you try restarting those two and see if it was just a fluke.

@skoudoro
Copy link
Member

Thanks ! I just restarted them

@skoudoro skoudoro merged commit f8cadac into dipy:master Jul 23, 2020
@skoudoro
Copy link
Member

merging, Thank you @grlee77!

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.

None yet

3 participants