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

RF: Create PCA denoising utils methods #3001

Merged
merged 1 commit into from
Dec 10, 2023

Conversation

jhlegarreta
Copy link
Contributor

Create PCA denoising utils methods: refactor code blocks into their own methods to improve encapsulation.

Add the corresponding tests.

Add a method to report about the dimensionality problem message.

The above allows for the dimensionality problem message to be checked and filtered in tests.

Fixes:

denoise/tests/test_lpca.py::test_mppca_returned_sigma
 denoise/localpca.py:194: UserWarning:
 Number of samples 27 - 1 < Dimensionality 104.
 This might have a performance impact.
 Increase patch_radius to 2 to avoid this warning,
 or supply suppress_warning=True to your function call.
    warn(e_s, UserWarning)

reported, for example, at:
https://github.com/dipy/dipy/actions/runs/7146907000/job/19465502675#step:9:4410

@pep8speaks
Copy link

pep8speaks commented Dec 10, 2023

Hello @jhlegarreta, Thank you for updating !

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

Comment last updated at 2023-12-10 01:22:45 UTC

return np.prod(patch_size)


def compute_suggested_patch_radius(arr, patch_size):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could have taken arr.shape[-1] as the first argument, but I thought it was less clear from the point of view of the caller.

Create PCA denoising utils methods: refactor code blocks into their own
methods to improve encapsulation.

Add the corresponding tests.

Add a method to report about the dimensionality problem message.

The above allows for the dimensionality problem message to be checked
and filtered in tests.

Fixes:
```
denoise/tests/test_lpca.py::test_mppca_returned_sigma
 denoise/localpca.py:194: UserWarning:
 Number of samples 27 - 1 < Dimensionality 104.
 This might have a performance impact.
 Increase patch_radius to 2 to avoid this warning,
 or supply suppress_warning=True to your function call.
    warn(e_s, UserWarning)
```

reported, for example, at:
https://github.com/dipy/dipy/actions/runs/7146907000/job/19465502675#step:9:4410
Copy link

codecov bot commented Dec 10, 2023

Codecov Report

Merging #3001 (fe33ea9) into master (ca6268a) will increase coverage by 0.00%.
Report is 4 commits behind head on master.
The diff coverage is 87.50%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3001   +/-   ##
=======================================
  Coverage   81.71%   81.72%           
=======================================
  Files         147      147           
  Lines       20568    20577    +9     
  Branches     3279     3279           
=======================================
+ Hits        16808    16817    +9     
  Misses       2932     2932           
  Partials      828      828           
Files Coverage Δ
dipy/denoise/localpca.py 89.26% <87.50%> (+0.69%) ⬆️

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this refactoring.

Thanks @jhlegarreta, merging

@skoudoro skoudoro merged commit 88624ef into dipy:master Dec 10, 2023
31 checks passed
@jhlegarreta jhlegarreta deleted the FixPCAWarnings branch December 10, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants