Skip to content

Adding dual initialization for PDHG - #2169

Merged
MargaretDuff merged 9 commits into
TomographicImaging:masterfrom
hsw43:master
Jun 13, 2025
Merged

Adding dual initialization for PDHG#2169
MargaretDuff merged 9 commits into
TomographicImaging:masterfrom
hsw43:master

Conversation

@hsw43

@hsw43 hsw43 commented Jun 9, 2025 •
edited by MargaretDuff
Loading

Copy link
Copy Markdown
Contributor

Description

Adding dual initialization for PDHG

Example Usage

To initialize both primal and dual:
algo = PDHG(f=f,g=g,operator=A,tau=tau,sigma=sigma,update_objective_interval=1,initial=[x,y])

To only initialize primal:
algo = PDHG(f=f,g=g,operator=A,tau=tau,sigma=sigma,update_objective_interval=1,initial=x)
or
algo = PDHG(f=f,g=g,operator=A,tau=tau,sigma=sigma,update_objective_interval=1,initial=(x, None))

To only initialize dual:
algo = PDHG(f=f,g=g,operator=A,tau=tau,sigma=sigma,update_objective_interval=1,initial=(None, y))

❤️ Thanks for your contribution!

Changes

Testing you performed

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

Related issues/links

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

@MargaretDuff MargaretDuff self-assigned this Jun 10, 2025
@github-project-automation github-project-automation Bot moved this to Todo in CIL work Jun 10, 2025
@MargaretDuff

MargaretDuff commented Jun 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Many thanks for your contribution @hsw4! Initialising the dual will help make PDHG more flexible.

We chatted about this in the developers meeting and thought that we would like to retain the original behaviour that if you pass initial = customDataContainer it will initialise the primal. Thus we would like this logic:

  • If a DataContainer (or BlockDataContainer) is passed it is used to initialise the primal
  • If a tuple or list is passed then the first value is used for the primal, the second the dual (as you suggest). We would like that None could be passed in either position to give the default initialisation, for example [myCustomDataContainer, None] would initialise the primal but leave the dual as default and (None, myCustomDataContainer) would initialise the dual but leave the primal as default.

Are you happy to make this change or would you like us to do it?

@hsw43

hsw43 commented Jun 11, 2025

Copy link
Copy Markdown
Contributor Author

Sure, happy to make the change.

@hsw43

hsw43 commented Jun 11, 2025

Copy link
Copy Markdown
Contributor Author

I have just updated the code.
Ideally, I would like to check whether initial is a DataContainer in the first case. However, the algorithm failed to initialize when the initial is a SIRF ImageData. So I decided to go with a naive way of just checking whether initial is a tuple/ list.

@MargaretDuff

MargaretDuff commented Jun 12, 2025

Copy link
Copy Markdown
Member

Many thanks @hsw43 and good spot for the SIRF ImageData

lauramurgatroyd
lauramurgatroyd approved these changes Jun 12, 2025
View reviewed changes

@lauramurgatroyd lauramurgatroyd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great, thanks for your contribution!

Please can you add your name and institution to the top of each file you have edited.
An example of how to do this:

CIL/Wrappers/Python/cil/io/ZEISSDataReader.py

Lines 16 to 18 in eae0392

# Authors:
# CIL Developers, listed at: https://github.com/TomographicImaging/CIL/blob/master/NOTICE.txt
# Andrew Shartis (UES, Inc.)

One of us developers will also add you to the NOTICE.txt before merge

@hsw43
Added authors
8a17644 
Signed-off-by: hsw43 <[email protected]>
paskino
paskino reviewed Jun 12, 2025
View reviewed changes
Comment thread Wrappers/Python/cil/optimisation/algorithms/PDHG.py Outdated
paskino
paskino reviewed Jun 12, 2025
View reviewed changes
Comment thread Wrappers/Python/cil/optimisation/algorithms/PDHG.py Outdated
paskino
paskino reviewed Jun 12, 2025
View reviewed changes

@paskino paskino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm suggesting just a change in the docstring.

@MargaretDuff

MargaretDuff commented Jun 13, 2025

Copy link
Copy Markdown
Member

Many thanks @hsw43 for your contribution!

@MargaretDuff
MargaretDuff merged commit 0afee52 into TomographicImaging:master Jun 13, 2025
@github-project-automation github-project-automation Bot moved this from Todo to Done in CIL work Jun 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paskino paskino paskino left review comments
@lauramurgatroyd lauramurgatroyd lauramurgatroyd approved these changes

Assignees

@MargaretDuff MargaretDuff

Labels

None yet

Projects

CIL work
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hsw43 @MargaretDuff @paskino @lauramurgatroyd