-
Notifications
You must be signed in to change notification settings - Fork 60
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
Denoise nl_means script #972
base: master
Are you sure you want to change the base?
Conversation
Hello @EmmaRenauld, Thank you for updating ! There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2024-10-18 15:46:59 UTC |
1c52778
to
5c60070
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #972 +/- ##
==========================================
+ Coverage 68.73% 68.79% +0.06%
==========================================
Files 429 432 +3
Lines 22262 22498 +236
Branches 3322 3049 -273
==========================================
+ Hits 15301 15478 +177
- Misses 5674 5716 +42
- Partials 1287 1304 +17
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested all options and it works.
Since, all calls Dipy stuff, I feel ok with this. And we go back to what we had in the past with proper support of Rician noise and Piesno. Not perfect but can be quite useful in advance context.
Would love to make this PR go to the master quickly as it would be extremely useful for my IMN530 group.
Thanks! I'll check your comments soon. I was also wondering: I only used the parameters that were used in the bitbucket for piesno, but there are other parameters:
(See https://github.com/dipy/dipy/blob/master/dipy/denoise/noise_estimate.py) Should I add those parameters too? |
Good question @EmmaRenauld... not sure we should give access to these advanced parameters. I would not know how to tweak them myself... I guess an expert that would wish to play with this would probably write his own python script and call dipy directly, no? @arnaudbore thoughts? Ping me if you want to chat about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, quick comments
import scipy.stats | ||
|
||
|
||
def estimate_piesno_sigma(data, number_coils=0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this should goes into stats ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In scilpy.image
we have:
volume_math
: that's strictly the stuff from the script volume_mathvolume_operations
: it could go here, but it's not really an operation?
Or I create a scilpy.image.volume_metrics
?
Co-authored-by: Arnaud Bore <[email protected]>
Putting this here so that I don't forget this work in progress. See issue #939