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

Fix piesno type #1375

Merged
merged 4 commits into from Nov 22, 2017

Conversation

Projects
None yet
3 participants
@nilgoyette
Contributor

nilgoyette commented Nov 14, 2017

I found a problem with piesno. The line

np.sum(data**2, axis=-1, dtype=np.float32)

doesn't do what you think it does. For a dwi for example, the dtype is int16 and some values^2 are greater then 2^15.

The problem is that data**2 is still in int16, then it's converted to a float32 so this image is curiously bounded with the int16 bounds :)

Moreover, I changed axis=-1 to axis=2 because we always know the right axis... it's 2.

@arokem

This comment has been minimized.

Member

arokem commented Nov 14, 2017

Good catch. Would you mind writing a test that fails without this fix, so that we can avoid reintroducing it? I'd say keep axis=-1 for the cases where I feed in 2D arrays as data.

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 14, 2017

I'll see what I can do.

for the cases where I feed in 2D arrays as data.

# This method works on a 2D array with repetitions as the third dimension,
# so process the dataset slice by slice.
if data.ndim < 3:
    e_s = "This function only works on datasets of at least 3 dimensions."
    raise ValueError(e_s)

My change is in _piesno_3D which will always receive 3D data.

@arokem

This comment has been minimized.

Member

arokem commented Nov 14, 2017

Duh, of course. Thanks!

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 14, 2017

I don't know what to do with those 10 python 3.5 errors, they are all "ok".

Expected:
    array([ 1. ,  1.5,  2.5,  3.5,  4.5,  5. ])
Got:
    array([1. , 1.5, 2.5, 3.5, 4.5, 5. ])

Do you ignore them?

@codecov-io

This comment has been minimized.

codecov-io commented Nov 14, 2017

Codecov Report

Merging #1375 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   87.01%   87.02%   +<.01%     
==========================================
  Files         228      228              
  Lines       29086    29091       +5     
  Branches     3131     3132       +1     
==========================================
+ Hits        25310    25315       +5     
  Misses       3068     3068              
  Partials      708      708
Impacted Files Coverage Δ
dipy/denoise/noise_estimate.py 79.12% <100%> (ø) ⬆️
dipy/denoise/tests/test_noise_estimate.py 100% <100%> (ø) ⬆️

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 3d4a990...c4ad89b. Read the comment docs.

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 14, 2017

I tried to use the normal test_piesno image for the test but I always had the same answers. I created my own and took N=2 to see a difference.

@arokem

Did you mean to add the changes that you have in nlmeans? They seem unrelated to the fix.

def nlmeans(arr, sigma, mask=None, patch_radius=1, block_radius=5,
rician=True, num_threads=None):
rician=False, num_threads=None):

This comment has been minimized.

@arokem

arokem Nov 15, 2017

Member

Why did you change this default?

return denoised_arr
else:
raise ValueError("Only 3D or 4D array are supported!", arr.shape)
def main():

This comment has been minimized.

@arokem

arokem Nov 15, 2017

Member

Please remove this code. I think that is causing the current failures.

@@ -67,15 +59,49 @@ def nlmeans(arr, sigma, mask=None, patch_radius=1, block_radius=5,
sigma = np.ones(arr.shape, dtype=np.float64) * sigma
for i in range(arr.shape[-1]):
start = datetime.now()

This comment has been minimized.

@arokem

arokem Nov 15, 2017

Member

Please remove the timing code from here.

denoised_arr[..., i] = nlmeans_3d(arr[..., i],
mask,
sigma[..., i],
patch_radius,
block_radius,
rician,
num_threads).astype(arr.dtype)
print(datetime.now() - start)

This comment has been minimized.

@arokem

arokem Nov 15, 2017

Member

And this as well

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 15, 2017

Damn, sorry, my bad. These change are unrelated to the current topic. I did a commit -a without thinking. Will remove tomorrow at work.

@arokem

This comment has been minimized.

Member

arokem commented Nov 16, 2017

Hi @nilgoyette : the failures you see on the last bot are probably unrelated to your work here. This bot tests new releases of numpy/scipy before they go live, so that we can fix things in advance. We'll need to fix these on a separate PR, before we can merge yours (which I believe is fine).

@arokem

This comment has been minimized.

Member

arokem commented Nov 18, 2017

OK - now that #1377 is merged, this failure should go away.

Could you please rebase your branch on top of master?

@nilgoyette nilgoyette force-pushed the nilgoyette:fix_piesno_type branch from 547a449 to b8f48a8 Nov 20, 2017

@nilgoyette

This comment has been minimized.

Contributor

nilgoyette commented Nov 21, 2017

I did a rebase/push as suggested but it doesn't seem to start the CI. Is there a way to force a tests-run? Or maybe it's simply still failing.

@arokem

This comment has been minimized.

Member

arokem commented Nov 21, 2017

Maybe try again? I just merged #1378, so if you rebase again, that might sort this one out.

@nilgoyette nilgoyette force-pushed the nilgoyette:fix_piesno_type branch from b8f48a8 to c4ad89b Nov 21, 2017

@arokem

This comment has been minimized.

Member

arokem commented Nov 22, 2017

Off we go! Thanks!

@arokem arokem merged commit e06103e into nipy:master Nov 22, 2017

3 checks passed

codecov/patch 100% of diff hit (target 87.01%)
Details
codecov/project 87.02% (+<.01%) compared to 3d4a990
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

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