Expand patch radius if input is int

[FelixLiu-SF]

if patch radius is an int, it needs to be expanded to (1,3) array before calling _extract_3d_patches.

[pep8speaks]

Hello @FelixLiu-SF, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated at 2021-04-21 20:47:26 UTC

[skoudoro]

Hi @FelixLiu-SF,

Thank you for doing this fix!

• Can you update the unit test also?
• Can you fix the pep8 above?

@ShreyasFadnavis, Can you look into this PR? Thank you

[ShreyasFadnavis]

Hi @FelixLiu-SF - Good catch! And thank you for this PR 👍🏽

1 minor comment from me, otherwise I think the PR looks good!

@skoudoro I don't think there is a unit test for this functionality.

[ShreyasFadnavis]

Maybe hard to unit-test this since it's not a function of its own. We can make a function for it if necessary!

[skoudoro]

Maybe hard to unit-test this since it's not a function of its own. We can make a function for it if necessary!

In your unit tests, you call many times patch2selfwith the default patch_radius. You just need to pick one of them and add patch_radius=0 instead of [0, 0, 0]. It should be enough, we should get the same result and the int is tested

[ShreyasFadnavis]

Maybe hard to unit-test this since it's not a function of its own. We can make a function for it if necessary!

In your unit tests, you call many times patch2selfwith the default patch_radius. You just need to pick one of them and add patch_radius=0 instead of [0, 0, 0]. It should be enough, we should get the same result and the int is tested

Yep -- Good idea! Works :)

[skoudoro]

Hi @FelixLiu-SF,

Thank you for your pep8 fix. Can you update a test and then your PR will be ready to be merged.

You just need to update this line and add patch_radius=0 as a parameter.

Thank you.

[codecov]

Codecov Report

Merging #2372 (f6089b5) into master (e96c440) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2372      +/-   ##
==========================================
- Coverage   85.25%   85.24%   -0.01%     
==========================================
  Files         125      125              
  Lines       16556    16558       +2     
  Branches     2683     2684       +1     
==========================================
+ Hits        14114    14115       +1     
  Misses       1759     1759              
- Partials      683      684       +1     

Impacted Files	Coverage Δ
dipy/denoise/patch2self.py	73.63% <100.00%> (+0.48%)	⬆️
dipy/reconst/forecast.py	89.84% <0.00%> (-0.51%)	⬇️

[FelixLiu-SF]

Hi @FelixLiu-SF,

Thank you for your pep8 fix. Can you update a test and then your PR will be ready to be merged.

You just need to update this line and add patch_radius=0 as a parameter.

Thank you.

All set!

[ShreyasFadnavis]

Thanks for this @FelixLiu-SF ! I think its ready to be merged 👍🏽

I will wait till tomorrow if anyone else has any other comments.
Thanks for the review @skoudoro!

[ShreyasFadnavis]

Okay! This is good to go! Merging 🚀

Thanks @FelixLiu-SF !