Fix kwargs in PDHG

[MargaretDuff]

Changes

• check_convergence was a kwarg that, if passed, was then passed to the algorithm parent class which raised an error because it wasn't recognised
• Similarly, you couldn't pass the theta kwarg because, if passed, it was then passed to the algorithm parent class which raised an error because it wasn't recognised

Discussed with Gemma and agreed to pop these kwargs before passing to the parent class

It is still open to me whether check_convergence should default to True or False. In the original issue, I claimed it should be False by default, but now I am not so sure. Mathematical convergence does not guarantee that it converges computationally in any reasonable time frame, and we know that we can get good (and sometimes quicker) solutions outside the mathematical convergence guarantees.

We decided to default to True to match with the docstring.

Testing you performed

Please add any demo scripts to https://github.com/TomographicImaging/CIL-Demos/tree/main/misc

Related issues/links

Closes #1965

Checklist

• I have performed a self-review of my code
• I have added docstrings in line with the guidance in the developer guide
• I have updated the relevant documentation
• I have implemented unit tests that cover any new or modified functionality
• CHANGELOG.md has been updated with any functionality change
• Request review from all relevant developers
• Change pull request label to 'Waiting for review'

Contribution Notes

Please read and adhere to the developer guide and local patterns and conventions.

• The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in CIL (the Work) under the terms and conditions of the Apache-2.0 License
• I confirm that the contribution does not violate any intellectual property rights of third parties

[lauramurgatroyd]

Sounds like current master defaults to check_convergence being False (as you say it throws an error if its passed)? I would choose the default that preserves current behaviour rather than breaking backward compatability

[MargaretDuff]

Sounds like current master defaults to check_convergence being False (as you say it throws an error if its passed)? I would choose the default that preserves current behaviour rather than breaking backward compatability

The current approach runs it as default, but I think you can't turn it off as passing check_convergence=False leads to the error. To conserve backwards compatibility and to match the docstring, have left the default as True

[lauramurgatroyd]

No worries if docs suggestions are out of scope - my main concern is the update_objective_interval

[lauramurgatroyd]

can confirm the todo is gone!

Let's go 🥳

[gfardell]

looks good!