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

faster version of piesno #745

Merged
merged 5 commits into from Dec 13, 2015

Conversation

Projects
None yet
3 participants
@samuelstjean
Contributor

samuelstjean commented Oct 22, 2015

Some small improvements I had on my version, should be 80 to 100 times faster.

@@ -201,6 +201,12 @@ def _piesno_3D(data, N, alpha=0.01, l=100, itermax=100, eps=1e-5,
Journal of Magnetic Resonance 2009; 197: 108-119.
"""
if np.all(data == 0):

This comment has been minimized.

@arokem

arokem Oct 24, 2015

Member

Please write a test for this code path.

This comment has been minimized.

@arokem

arokem Oct 24, 2015

Member

Good call though - GIGO

@arokem

This comment has been minimized.

arokem commented on dipy/denoise/noise_estimate.py in ba606b2 Oct 24, 2015

Is there some reason to index into mask with an ellipsis?

@arokem

This comment has been minimized.

Member

arokem commented Oct 24, 2015

in general, testing in this module is rather spotty (76%). Please add tests of corner cases, to be sure that we actually exercise these.

@arokem

This comment has been minimized.

Member

arokem commented Oct 24, 2015

Other than that looks good. I tried this with the example, to make sure we don't get any regressions, and it does replicate exactly the same results as before, and runs much faster - nice work.

@arokem

This comment has been minimized.

Member

arokem commented Nov 7, 2015

Any more thoughts here? @samuelstjean - did you see my comments?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Nov 8, 2015

Yes, but since the mask is recomputed at each iteration, I dont know if
it's better to replace or reassign the values. And I don't know the
difference between : and ... for this case.
On Nov 7, 2015 22:36, "Ariel Rokem" notifications@github.com wrote:

Any more thoughts here? @samuelstjean https://github.com/samuelstjean -
did you see my comments?


Reply to this email directly or view it on GitHub
#745 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Nov 8, 2015

Yep. Neither do I, which is why I asked. Does anyone else have an opinion
about this?

@samuelstjean: have you had a chance to get some more tests in there?

On Sun, Nov 8, 2015 at 3:58 AM, Samuel St-Jean notifications@github.com
wrote:

Yes, but since the mask is recomputed at each iteration, I dont know if
it's better to replace or reassign the values. And I don't know the
difference between : and ... for this case.
On Nov 7, 2015 22:36, "Ariel Rokem" notifications@github.com wrote:

Any more thoughts here? @samuelstjean https://github.com/samuelstjean

did you see my comments?


Reply to this email directly or view it on GitHub
#745 (comment).


Reply to this email directly or view it on GitHub
#745 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 8, 2015

It looks good. @samuelstjean increase the coverage and let's merge this asap.

@arokem

This comment has been minimized.

Member

arokem commented Nov 8, 2015

If not, it can also wait for after the release.

On Sun, Nov 8, 2015 at 2:42 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

It looks good. @samuelstjean https://github.com/samuelstjean increase
the coverage and let's merge this asap.


Reply to this email directly or view it on GitHub
#745 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Nov 8, 2015

I mean for the next release.

On Sun, Nov 8, 2015 at 2:47 PM, Ariel Rokem arokem@gmail.com wrote:

If not, it can also wait for after the release.

On Sun, Nov 8, 2015 at 2:42 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

It looks good. @samuelstjean https://github.com/samuelstjean increase
the coverage and let's merge this asap.


Reply to this email directly or view it on GitHub
#745 (comment).

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Nov 9, 2015

Busy ismrm stuff this week.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Nov 20, 2015

How do you check coverage by the way? Coveralls.io seems ok for reporting that automatically.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 20, 2015

You can use

nosetests --with-coverage --cover-package=dipy.denoise

You will need to pip install coverage.
Also Travis reports coverage automatically in specific bots which have the flag COVERAGE=1
See here
https://travis-ci.org/nipy/dipy/jobs/92228117

sig_prev = sig
# If no point meets the criterion, exit
if omega.size == 0:
break

This comment has been minimized.

@arokem

arokem Dec 10, 2015

Member

This one's also not tested at the moment.

This comment has been minimized.

@Garyfallidis

Garyfallidis Dec 13, 2015

Member

@samuelstjean and @arokem is this now tested?

This comment has been minimized.

@samuelstjean

samuelstjean Dec 13, 2015

Contributor

Yes thats the latest commit I added
On Dec 13, 2015 02:44, "Eleftherios Garyfallidis" notifications@github.com
wrote:

In dipy/denoise/noise_estimate.py
#745 (comment):

  •        sig_prev = sig
    
  •    # If no point meets the criterion, exit
    
  •    if omega.size == 0:
    
  •        break
    

@samuelstjean https://github.com/samuelstjean and @arokem
https://github.com/arokem is this now tested?


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/745/files#r47439250.

@arokem

This comment has been minimized.

Member

arokem commented Dec 10, 2015

Would you mind rebasing this on master? That way, we'll get it also tested on 3.5 on travis. Thanks.

@samuelstjean samuelstjean force-pushed the samuelstjean:faster_piesno branch from da4d661 to 8a87011 Dec 10, 2015

Garyfallidis added a commit that referenced this pull request Dec 13, 2015

@Garyfallidis Garyfallidis merged commit 11b0ddb into nipy:master Dec 13, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@samuelstjean samuelstjean deleted the samuelstjean:faster_piesno branch Dec 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment