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

MAINT: Skip the check for theta <= 1 in the PDHG #1637

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sbanert
Copy link
Contributor

@sbanert sbanert commented Feb 13, 2024

DOC: Update the PDHG docstring to latest research.

The Chambolle-Pock method converges even for $\theta &gt; 1$, so the check that requires the contrary (and aborts with an error otherwise) should be removed. I also updated the docstring with a paper of ours that shows relaxed requirements on the step lengths.

Moreover, I removed certain statements that seem unnecessary from the docstring like the requirement that the spaces be finite-dimensional (for infinite-dimensional spaces, the convergence is weak, as is standard for these methods), and that the functionals be non-negative (this is definitely not needed).

I am not even sure if it is good to keep the check for non-negativity of $\theta$ in the code. If users try to experiment with negative $\theta$, why not? Our code should not break in this case. The check for $\theta \leq 1$ currently prevents us from using this method for numerical experiments for the paper [BUG2023].

@pep8speaks
Copy link

pep8speaks commented Feb 13, 2024

Checking updated PR...

No PEP8 issues.

Comment last updated at 2024-02-13 18:45:18 UTC

DOC: Update the PDHG docstring to latest research.
@ozanoktem
Copy link
Contributor

Agree that the solver should in general not prevent a user to call it with parameter settings that does not guarantee convergence. Perhaps it is enough that the solver raises a warning when it is used in this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants