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

Explicit TGV #1986

Closed
wants to merge 9 commits into from
Closed

Explicit TGV #1986

wants to merge 9 commits into from

Conversation

paskino
Copy link
Collaborator

@paskino paskino commented Nov 29, 2023

Issue

closes #1985

Description

Implementation of a Total Generalised Variation regulariser for Least Squares problems. It works both with PDHG and the SPDHG algorithms.

Controls to from the interface that are required:

  1. alpha, regularisation parameter as in Eq. 3.5 https://royalsocietypublishing.org/doi/10.1098/rsta.2020.0193
  2. gamma, ratio beta/alpha, wrt to Eq. 3.5 https://royalsocietypublishing.org/doi/10.1098/rsta.2020.0193
  3. regulariser, whether TV or TGV

Add a description of the changes made.

Testing

Describe the tests that were used to verify your changes.

Acceptance Criteria

How should the reviewer test your changes?

Documentation

How have you changed the documentation to reflect your changes? All changes should be noted in the appropriate file in docs/release_notes

@paskino
Copy link
Collaborator Author

paskino commented Nov 30, 2023

I believe this code is ready to be tested by you @samtygier-stfc @JackEAllen

@JackEAllen
Copy link
Collaborator

I believe this code is ready to be tested by you @samtygier-stfc @JackEAllen

Thanks, I'll take a look.

@samtygier-stfc
Copy link
Collaborator

@paskino Could you cherry-pick 746e96c faae8eb and bbd84dd from https://github.com/mantidproject/mantidimaging/commits/paskino_explicit_TGV. That should get the tests passing. Thanks

@samtygier-stfc
Copy link
Collaborator

Thanks that looks like it solves the non-negative part. The other issues was when stochastic is enabled:

2023-12-05 14:48:19,767 [mantidimaging.gui.dialogs.async_task.task:L53] ERROR: Failed to execute task: tuple index out of range
Traceback (most recent call last):
  File "/home/sam/git/mantidimaging/mantidimaging/gui/dialogs/async_task/task.py", line 50, in run
    self.result = call_with_known_parameters(self.task_function, **self.kwargs)
  File "/home/sam/git/mantidimaging/mantidimaging/core/utility/func_call.py", line 12, in call_with_known_parameters
    return func(**ka)
  File "/home/sam/git/mantidimaging/mantidimaging/gui/windows/recon/model.py", line 144, in run_preview_recon
    recon.data[0] = reconstructor.single_sino(images.sino(slice_idx),
  File "/home/sam/git/mantidimaging/mantidimaging/core/reconstruct/cil_recon.py", line 249, in single_sino
    algo = SPDHG(f=F,
  File "/home/sam/mambaforge/envs/mantidimaging-dev/lib/python3.10/site-packages/cil/optimisation/algorithms/SPDHG.py", line 105, in __init__
    self.set_up(f=f, g=g, operator=operator, tau=tau, sigma=sigma,
  File "/home/sam/mambaforge/envs/mantidimaging-dev/lib/python3.10/site-packages/cil/optimisation/algorithms/SPDHG.py", line 156, in set_up
    norms = [operator.get_item(i,0).norm() for i in range(self.ndual_subsets)]
  File "/home/sam/mambaforge/envs/mantidimaging-dev/lib/python3.10/site-packages/cil/optimisation/algorithms/SPDHG.py", line 156, in <listcomp>
    norms = [operator.get_item(i,0).norm() for i in range(self.ndual_subsets)]
  File "/home/sam/mambaforge/envs/mantidimaging-dev/lib/python3.10/site-packages/cil/optimisation/operators/BlockOperator.py", line 136, in get_item
    return self.operators[index]
IndexError: tuple index out of range

@paskino
Copy link
Collaborator Author

paskino commented Dec 5, 2023

@samtygier-stfc I need to think about how to make SPDHG work. Can it be disabled for now?

@samtygier-stfc
Copy link
Collaborator

samtygier-stfc commented Dec 5, 2023

Thanks. Ok. I'll merge as is, and will disable stochastic for TGV when I do the GUI.

Current test failures look like the applitools API is down, its a secrets issue. so I'll worry about that.

@samtygier-stfc
Copy link
Collaborator

closing as we have #1997

Copy link
Collaborator

@MikeSullivan7 MikeSullivan7 left a comment

Choose a reason for hiding this comment

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

Looks good, seems to match up with the paper and give good results with a higher number of iterations

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.

Add TGV regularised LeastSquares reconstruction
4 participants