faster version of piesno #745
Conversation
| @@ -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): | |||
There was a problem hiding this comment.
Please write a test for this code path.
|
in general, testing in this module is rather spotty (76%). Please add tests of corner cases, to be sure that we actually exercise these. |
|
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. |
|
Any more thoughts here? @samuelstjean - did you see my comments? |
|
Yes, but since the mask is recomputed at each iteration, I dont know if
|
|
Yep. Neither do I, which is why I asked. Does anyone else have an opinion @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
|
|
It looks good. @samuelstjean increase the coverage and let's merge this asap. |
|
If not, it can also wait for after the release. On Sun, Nov 8, 2015 at 2:42 PM, Eleftherios Garyfallidis <
|
|
I mean for the next release. On Sun, Nov 8, 2015 at 2:47 PM, Ariel Rokem arokem@gmail.com wrote:
|
|
Busy ismrm stuff this week. |
|
How do you check coverage by the way? Coveralls.io seems ok for reporting that automatically. |
|
You can use You will need to pip install coverage. |
| sig_prev = sig | ||
| # If no point meets the criterion, exit | ||
| if omega.size == 0: | ||
| break |
There was a problem hiding this comment.
This one's also not tested at the moment.
There was a problem hiding this comment.
@samuelstjean and @arokem is this now tested?
There was a problem hiding this comment.
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, exitif 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.
|
Would you mind rebasing this on master? That way, we'll get it also tested on 3.5 on travis. Thanks. |
samuelstjean
force-pushed
the
faster_piesno
branch
from
da4d661
to
8a87011
Compare
Some small improvements I had on my version, should be 80 to 100 times faster.