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

Adds control for number of iterations in CSD recon #1849

Merged
merged 4 commits into from Jul 17, 2019

Conversation

@scott-trinkle
Copy link
Contributor

commented May 29, 2019

This allows the user to pass the maximum number of iterations as an argument when setting up a CSD model. This functionality already existed in the csdeconv function, but was not accessible with a standard workflow of setting up a CSD model object, then calling the fit method.

@arokem

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Looks good to me. Any chance you could add a test that exercises this? At the very least a smoke-test would be good to have. Thanks!

@codecov-io

This comment has been minimized.

Copy link

commented May 29, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@17b68f0). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1849   +/-   ##
========================================
  Coverage          ?   85.4%           
========================================
  Files             ?     119           
  Lines             ?   14310           
  Branches          ?    2249           
========================================
  Hits              ?   12222           
  Misses            ?    1577           
  Partials          ?     511
Impacted Files Coverage Δ
dipy/reconst/csdeconv.py 89.93% <100%> (ø)

model = ConstrainedSphericalDeconvModel(gtab, (evals[0], 3.),
sh_order=8, convergence=50)

This comment has been minimized.

Copy link
@arokem

arokem May 29, 2019

Member

Sorry - could you add a fit call in here? Just to check that it propagates through properly.

This comment has been minimized.

Copy link
@scott-trinkle

scott-trinkle May 29, 2019

Author Contributor

ah, good point - will do!

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 1, 2019

Member

It would be good to have an assert somewhere on this test.

@arokem

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@skoudoro
Copy link
Member

left a comment

Thank you for this @scott-trinkle. #1844 has been fixed so can you rebase this PR? I added a small comment below, this PR is almost ready to go!


model = ConstrainedSphericalDeconvModel(gtab, (evals[0], 3.),
sh_order=8, convergence=50)

This comment has been minimized.

Copy link
@skoudoro

skoudoro Jul 1, 2019

Member

It would be good to have an assert somewhere on this test.

@scott-trinkle scott-trinkle force-pushed the scott-trinkle:add_iter_to_CSD branch from 100c182 to 97edac6 Jul 16, 2019

@skoudoro skoudoro merged commit 5196d0b into nipy:master Jul 17, 2019

5 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Thank you @scott-trinkle!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.