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

fixed pca for features > samples, and fixed pca_noise_estimate #2722

Merged
merged 1 commit into from Mar 22, 2023

Conversation

samcoveney
Copy link
Contributor

Following on from #2678 I have attempted to do several things:

  1. fix genpca to work when the number of samples (voxels) is less than the number of features (images) by modifying _pca_classifier
  2. fix pca_noise_estimate to be able to use the canonical algorithm

This is a new pull request to replace #2707 which became totally corrupted for reasons I don't understand, and I simply could not fix. Sorry about that. But I addressed all comments left at the time.

@samcoveney
Copy link
Contributor Author

samcoveney commented Feb 23, 2023

Hi, I opened this around 3 weeks ago, with the original corrupted pull on 4 Jan, will it be possible to assign someone to this soon please? Again, very sorry to have needed to close the original pull because I really messed something up with git and couldn't figure it out. @jhlegarreta was helping me before my error (I implemented the suggestions).

@Garyfallidis
Copy link
Contributor

Thank you @samcoveney I have assigned @ShreyasFadnavis to check your PR. You should hear from him asap. Thank you!

@skoudoro
Copy link
Member

No worries for closing the previous one!

Apologize for the long wait, I tried to ping some expert in denoising but no feedback until now.

Overall, it looks good to me. I need to test a last time to refresh my memory since since it is was long time ago.

@arokem, @ garyfallidis, any opinions?

@samcoveney
Copy link
Contributor Author

Thanks everyone! Looking forward to comments

Copy link
Contributor

@RafaelNH RafaelNH left a comment

Choose a reason for hiding this comment

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

Hi @samcoveney! Thanks for your work on this! Only have 4 minor comments that I would like you to addressed. After that, I am happy that your PR is merged.

dipy/denoise/localpca.py Outdated Show resolved Hide resolved
dipy/denoise/localpca.py Outdated Show resolved Hide resolved
dipy/denoise/pca_noise_estimate.pyx Show resolved Hide resolved
dipy/denoise/pca_noise_estimate.pyx Show resolved Hide resolved
@samcoveney
Copy link
Contributor Author

samcoveney commented Mar 10, 2023

I have addressed the comments / changes and squashed everything. Thanks!
edit: then re-squashed after fixing spelling mistakes, sorry

@RafaelNH
Copy link
Contributor

Thanks @samcoveney ! I don't have anything to add.

@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Merging #2722 (e928a82) into master (9393354) will increase coverage by 0.10%.
The diff coverage is 85.71%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2722      +/-   ##
==========================================
+ Coverage   83.83%   83.93%   +0.10%     
==========================================
  Files         131      132       +1     
  Lines       18293    18466     +173     
  Branches     2980     3015      +35     
==========================================
+ Hits        15336    15500     +164     
- Misses       2220     2222       +2     
- Partials      737      744       +7     
Impacted Files Coverage Δ
dipy/denoise/localpca.py 88.57% <81.81%> (-0.91%) ⬇️
dipy/reconst/dti.py 95.48% <100.00%> (ø)
dipy/tracking/streamline.py 93.83% <100.00%> (ø)

... and 5 files with indirect coverage changes

@RafaelNH
Copy link
Contributor

Hi @samcoveney!
Actually I have a last comment. Since you are changing already existing DIPY procedures, can you report your changes in the dipy api_change documentation? Please create a new section for DIPY 1.7.0 and add a description of your changes there. Many thanks!

@skoudoro
Copy link
Member

Thank you @RafaelNH and @jhlegarreta for the review.

Thank you @samcoveney for this PR.

I will wait until monday/tuesday to merge this PR, just in case there are any additional comments

@skoudoro skoudoro merged commit 77afd60 into dipy:master Mar 22, 2023
23 checks passed
@skoudoro
Copy link
Member

Thank you for this work @samcoveney and sorry for the delay, I believed it was merged before the break....

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

4 participants