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

[WIP] Random lpca #1781

Closed
wants to merge 164 commits into from

Conversation

@15thai
Copy link

commented Mar 15, 2019

New Implementation - Random matrix local pca.
References
----------
.. [Veraart16] Veraart J, Fieremans E, Novikov DS (2016)
Diffusion MRI noise mapping using random matrix theory.
Magnetic resonance in Medicine 76(5), p1582-1593.
https://doi.org/10.1002/mrm.26059
"""

15thai and others added 20 commits Nov 14, 2018
Change "is" check for 'GCV'
This is causing an error in python 3.7
Merge pull request #1769 from mattcieslak/patch-1
Change "is" check for 'GCV'
fast_matvec('n',C[k,:,:], temp0[k,:], temp1[k,:])
fast_matvec('n',X[k,:,:], temp1[k,:], temp2[k,:])

denoised_arr_view[i,j,k,:] = temp2[k,:]

This comment has been minimized.

Copy link
@RafaelNH

RafaelNH Mar 16, 2019

Contributor

You are only using the center voxel of each patch for denoising. Please consider using all voxels like using an overcomplete average approach (Manjon et al., 2013) In this way, we avoid removing information in cases that tissues are near to the limits of the image or in cases that patch size is large. Keep in mind that some researcher might use PCA in all voxels of an image simultaneously

@cython.wraparound(False)
@cython.boundscheck(False)
@cython.nonecheck(False)
def randommatrix_localpca_parallel(arr, patch_extent=0, out_dtype=None,num_threads=None):

This comment has been minimized.

Copy link
@RafaelNH

RafaelNH Mar 16, 2019

Contributor

Can you remake this function to something shorter? Suggestion: pca_denoise.

This comment has been minimized.

Copy link
@ShreyasFadnavis

ShreyasFadnavis Mar 20, 2019

Member

Hi Anh! I agree with @RafaelNH on this one, I would suggest randommatrix_lpca.pyx

@RafaelNH

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Hi @15thai! Very nice start!
See my two comments in the code. In addition to those, please reformat the code using PEP8 style. Do not forget to add tests and an example for the code usage. Keep the good work!

@codecov-io

This comment has been minimized.

Copy link

commented Mar 17, 2019

Codecov Report

Merging #1781 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1781   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files         120      120           
  Lines       14572    14572           
  Branches     2295     2295           
=======================================
  Hits        12224    12224           
  Misses       1824     1824           
  Partials      524      524
scott-trinkle and others added 5 commits Mar 18, 2019
skoudoro and others added 9 commits May 16, 2019
Merge pull request #1842 from skoudoro/remove-tput
[Fix] Remove tput from fetcher
randommatrix_lpca version
I wrote a python version to work with the test. But I am not sure if I can use the same test for lpca. 
Let me know
@pep8speaks

This comment has been minimized.

Copy link

commented May 28, 2019

Hello @15thai, Thank you for updating !

Line 6:38: E251 unexpected spaces around keyword / parameter equals
Line 6:40: E251 unexpected spaces around keyword / parameter equals
Line 12:81: E501 line too long (82 > 80 characters)
Line 21:42: W605 invalid escape sequence '\l'
Line 24:81: E501 line too long (81 > 80 characters)
Line 30:81: E501 line too long (82 > 80 characters)
Line 76:81: E501 line too long (114 > 80 characters)
Line 77:81: E501 line too long (85 > 80 characters)
Line 81:43: E202 whitespace before ')'
Line 83:21: E128 continuation line under-indented for visual indent
Line 84:21: E128 continuation line under-indented for visual indent
Line 116:81: E501 line too long (90 > 80 characters)
Line 133:81: E501 line too long (107 > 80 characters)
Line 135:81: E501 line too long (119 > 80 characters)

Comment last updated at 2019-06-14 21:32:36 UTC
15thai and others added 16 commits May 29, 2019
DOC: Add single-module test/coverage instructions
This commit adds instructions for running tests and coverage reports for
a single module without running the entire DIPY test suite. I found this
useful for my first DIPY contributions since I was running into a lot of
test errors for parts of the code base that I hadn't touched. These
instructions might be more inviting to first-time contributors.
Merge pull request #1851 from richford/doc-test-instructions
DOC: Add single-module test/coverage instructions
Merge pull request #1836 from clintg6/master
Corrected median_otsu function declaration that was breaking tutorials
Merge pull request #1835 from skoudoro/fix-1764
[Fix]  Workflow mask documentation
Pin scipy version for bots that need statsmodels.
This is due to breaking changes in the scipy API that will be fixed on
an eventual release of statsmodels. For now, let's pin this and unpin
when a new release of scipy comes along.
Merge pull request #1855 from arokem/pin_scipy_for_statsmodels
Pin scipy version for bots that need statsmodels.
@ShreyasFadnavis

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@15thai something went wrong with Git! Can you try to revert to commit ce41956? Or rebase maybe?
@skoudoro can you check this?

@skoudoro

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

I will try asap

@15thai

This comment has been minimized.

Copy link
Author

commented Jun 20, 2019

I misunderstood the merging my local to my branch. What should I do?

@15thai 15thai closed this Jun 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.