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

Denoise nl_means script #972

Merged
merged 9 commits into from
Nov 8, 2024
Merged

Conversation

EmmaRenauld
Copy link
Contributor

Putting this here so that I don't forget this work in progress. See issue #939

@pep8speaks
Copy link

pep8speaks commented Apr 16, 2024

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-31 12:33:04 UTC

@arnaudbore arnaudbore added the WIP Work In Progress label Jun 18, 2024
Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 69.86301% with 22 lines in your changes missing coverage. Please review.

Project coverage is 68.79%. Comparing base (9d94226) to head (38415f3).
Report is 82 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
+ Coverage   68.73%   68.79%   +0.05%     
==========================================
  Files         429      433       +4     
  Lines       22262    22500     +238     
  Branches     3322     3049     -273     
==========================================
+ Hits        15301    15478     +177     
- Misses       5674     5717      +43     
- Partials     1287     1305      +18     
Components Coverage Δ
Scripts 69.63% <64.91%> (-0.02%) ⬇️
Library 67.59% <87.50%> (+0.16%) ⬆️

Copy link
Contributor

@mdesco mdesco left a 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.

scripts/scil_denoising_nlmeans.py Outdated Show resolved Hide resolved
scripts/scil_denoising_nlmeans.py Show resolved Hide resolved
scripts/scil_denoising_nlmeans.py Show resolved Hide resolved
scripts/scil_denoising_nlmeans.py Show resolved Hide resolved
@EmmaRenauld
Copy link
Contributor Author

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:

alpha=0.01, step=100, itermax=100, eps=1e-5

(See https://github.com/dipy/dipy/blob/master/dipy/denoise/noise_estimate.py)

Should I add those parameters too?

@mdesco
Copy link
Contributor

mdesco commented Sep 25, 2024

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.

Copy link
Contributor

@arnaudbore arnaudbore left a comment

Choose a reason for hiding this comment

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

LGTM, quick comments

scripts/tests/test_denoising_nlmeans.py Outdated Show resolved Hide resolved
scilpy/stats/stats.py Outdated Show resolved Hide resolved
@EmmaRenauld EmmaRenauld changed the title [WIP] Denoise nl_means script Denoise nl_means script Oct 18, 2024
@arnaudbore arnaudbore merged commit 7501322 into scilus:master Nov 8, 2024
2 checks passed
@EmmaRenauld EmmaRenauld deleted the denoise_scripts branch November 11, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work In Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants