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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify the meaning of the weights term for TV denoising functions #6134

Closed
wants to merge 2 commits into from

Conversation

gmelon
Copy link
Contributor

@gmelon gmelon commented Dec 18, 2021

Description

Fixed #5379

After discovering above issue, I modified the docstring to clarify the meaning of weight a little more.
Please review it. Thanks you馃檪

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

@pep8speaks
Copy link

pep8speaks commented Dec 18, 2021

Hello @gmelon! Thanks for updating this PR. We checked the lines you've touched for PEP聽8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 馃嵒

Comment last updated at 2021-12-18 17:44:49 UTC

@mkcor mkcor added the 馃搫 type: Documentation Updates, fixes and additions to documentation label Dec 19, 2021
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, @gmelon.

As is, the only addition brought by this changeset is saying that 'more denoising' means 'more smoothing' -- it's not addressing #5379 (and since a noisy signal is notably not smooth, the reader already understands intuitively that 'more denoising' means 'more smoothing').

In the original issue, @grlee77 pointed out that what needs clarifying is which term in the cost function the weight applies to. And I would argue that the double negative form "the expense of less similarity to the input" calls for rewording.

@gmelon
Copy link
Contributor Author

gmelon commented Dec 27, 2021

Thanks for your contribution, @gmelon.

As is, the only addition brought by this changeset is saying that 'more denoising' means 'more smoothing' -- it's not addressing #5379 (and since a noisy signal is notably not smooth, the reader already understands intuitively that 'more denoising' means 'more smoothing').

In the original issue, @grlee77 pointed out that what needs clarifying is which term in the cost function the weight applies to. And I would argue that the double negative form "the expense of less similarity to the input" calls for rewording.

@mkcor, Thank you for the comments. In summary, do you mean that "the expense of less simplicity to the input" needs to be modified in some way?

@grlee77
Copy link
Contributor

grlee77 commented Dec 29, 2021

Ideally, we would add the actual cost functions using LaTeX math markup similarly to the example here:

The normalized mutual information of :math:`A` and :math:`B` is given by::
..math::
Y(A, B) = \frac{H(A) + H(B)}{H(A, B)}
where :math:`H(X) := - \sum_{x \in X}{x \log x}` is the entropy.

The user can then see the equation that is being minimized and where the weight occurs in it

Both functions are solving a similar problem, but the equation is formulated a bit differently in the two publications. If I recall correctly, in one of them the weight enforces similarity to the existing image whereas in the other the the weight is applied to the TV-regularization term in the equation. So, a larger weight in denoise_tv_chambolle gives a smoother image whereas for denoise_tv_bregman it is the opposite!

@grlee77
Copy link
Contributor

grlee77 commented Dec 29, 2021

I would use the following equation for denoise_tv_chambolle:

\min_{(x \in X)} \sum_{i=0}^{N-1} \left| \nabla{x_i} \right| + \frac{1}{2\lambda}\sum_{i=0}^{N-1} \left ( u_i - x_i \right )^2
which will render like:
eqn-white

The Bregman case can use the same equation, but with lambda in the numerator.

In the above u is the noisy image and x is the denoised one. The first term enforces smoothness while the second enforces consistency with the original image, u. The summations use a single, raveled, pixel index i to avoid making the equations specific for 2D images (singe the chambolle implementation is n-dimensional).

@mkcor
Copy link
Member

mkcor commented Jan 17, 2022

Thanks for your contribution, @gmelon.
As is, the only addition brought by this changeset is saying that 'more denoising' means 'more smoothing' -- it's not addressing #5379 (and since a noisy signal is notably not smooth, the reader already understands intuitively that 'more denoising' means 'more smoothing').
In the original issue, @grlee77 pointed out that what needs clarifying is which term in the cost function the weight applies to. And I would argue that the double negative form "the expense of less similarity to the input" calls for rewording.

@mkcor, Thank you for the comments. In summary, do you mean that "the expense of less simplicity to the input" needs to be modified in some way?

@gmelon no, I said three things, so they cannot be summarized into one. I said:

  • the changes as of 891836b don't add any value, so I would suggest reverting them;
  • the point is to clarify where the weight comes into play in the different functions to minimize; meanwhile, @grlee77 has provided much guidance on what to include;
  • yes, I would suggest rewording the double negative form "the expense of less similarity to the input."

Please let us know if you run into any roadblocks!

@alexdesiqueira
Copy link
Member

Hi @gmelon,
thank you for your work on this PR so far! Would you like to continue?
If not, we can help finishing it.
Thanks again!

@gmelon gmelon closed this Sep 23, 2022
@gmelon
Copy link
Contributor Author

gmelon commented Sep 23, 2022

Hi @gmelon,

thank you for your work on this PR so far! Would you like to continue?

If not, we can help finishing it.

Thanks again!

Thanks for asking!

I'm sorry to say that I can't continue this issue now.

Sorry again and Thanks you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
馃搫 type: Documentation Updates, fixes and additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clarify the meaning of the weights term for TV denoising functions
5 participants