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

revamp piesno docstring #1257

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@samuelstjean
Contributor

samuelstjean commented Jun 16, 2017

I was looking at finding why it does throw some divide by zero warning, even though they do not seem to affect the outcome. Seems like maybe when a slice has a lot of high values, the median is not zero but there might be no noise also, so I need to further check it out if it is just safe to squelch it instead.

For now some rewording and I need to also re-read the example to check it still makes sense, but I had this one for a few weeks already, so there is.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 16, 2017

Codecov Report

Merging #1257 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   87.04%   87.05%   +<.01%     
==========================================
  Files         226      226              
  Lines       28262    28262              
  Branches     3026     3026              
==========================================
+ Hits        24602    24603       +1     
  Misses       2976     2976              
+ Partials      684      683       -1
Impacted Files Coverage Δ
dipy/denoise/noise_estimate.py 79.12% <50%> (ø) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️

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 599af3e...1def437. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 1def437 on samuelstjean:fix_piesno_m_0 into 599af3e on nipy:master.

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage remained the same at 85.656% when pulling 1def437 on samuelstjean:fix_piesno_m_0 into 599af3e on nipy:master.

@arokem

arokem approved these changes Jun 16, 2017

LGTM. One small comment on my end.

@@ -149,10 +146,11 @@ def _piesno_3D(data, N, alpha=0.01, l=100, itermax=100, eps=1e-5,
-----------
data : ndarray
The magnitude signals to analyse. The last dimension must contain the
same realisation of the volume, such as dMRI or fMRI data.
same realisation of the volume.

This comment has been minimized.

@arokem

arokem Jun 16, 2017

Member

Maybe use the same wording as above? ("e.g. volumes collected during dMRI or fMRI acquisitions.")

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Jun 16, 2017

I still want to figure out why it does stuff like that before though

/home/travis/miniconda/envs/test-environment/lib/python3.5/site-packages/dipy/denoise/noise_estimate.py:248: RuntimeWarning: divide by zero encountered in true_divide
  s = sum_m2 / (2 * K * sigma**2)
/home/travis/miniconda/envs/test-environment/lib/python3.5/site-packages/dipy/denoise/noise_estimate.py:248: RuntimeWarning: invalid value encountered in true_divide
  s = sum_m2 / (2 * K * sigma**2)
/home/travis/miniconda/envs/test-environment/lib/python3.5/site-packages/dipy/denoise/noise_estimate.py:249: RuntimeWarning: invalid value encountered in less_equal
  mask[...] = np.logical_and(lambda_minus <= s, s <= lambda_plus)

probably nothing as it still works, but at least somehow squelching that would be useful.

@arokem

This comment has been minimized.

Member

arokem commented Jun 16, 2017

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Jun 16, 2017

That the symptom, I'm trying ot figure out why the std value goes to zero sometimes in the first place

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 19, 2017

I am sorry but this or any other PRs from Sam will not be merged as decided and expressed clearly last year. Sam there is no reason for you making any more PRs in dipy. We discussed about this last year let's not get back to the same discussions. I am closing this one and will remove PIESNO from the documentation. You can copy PIESNO to your own github project and work on improvements there.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Jun 19, 2017

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