Changes to admm and svmbir wrapper to address issue #57 #88

smajee · 2021-11-12T21:07:59Z

Addresses issue #57

Overview of changes (Mike and Soumendu):

Pass initializer to prox in admm
Use initial condition in svmbir prox
Make number of iterations in svmbir prox user-defined (with large number of iterations as default)
Make roi radius large by default in svmbir so that there is no masking and the solution matches with CG. (masking can be enabled by the user if required)
Use nanfix branch of svmbir in requirements (we can change it back once the fix is pushed to PyPi)

…main

…Loss uses this argument as well.

bwohlberg · 2021-11-12T21:14:05Z

Does this address all of the remaining check boxes (other than the PGM one) in #57?

smajee · 2021-11-12T21:19:22Z

Does this address all of the remaining check boxes (other than the PGM one) in #57?

This doesn't contain the tests for CG and SVMBIRWeightedSquaredL2Loss.

…prox_prox

bwohlberg · 2021-11-16T00:20:15Z

Recent changes in scico.loss (presumably the addition of the CG-based proxes) seems to have introduced quite a bit of untested code (see codecov report). It would be best to add the tests as part of this PR.

tbalke · 2021-11-16T01:02:21Z

Added some tests for the new proxes in

Recent changes in scico.loss (presumably the addition of the CG-based proxes) seems to have introduced quite a bit of untested code (see codecov report). It would be best to add the tests as part of this PR.

Added some tests for the proxes in 53713dd. Not enough.

tbalke · 2021-11-15T14:22:42Z

scico/linop/radon_svmbir.py

@@ -111,7 +143,9 @@ def _bproj_hcb(self, y):


 class SVMBIRWeightedSquaredL2Loss(WeightedSquaredL2Loss):


Docstring missing?

Yes. That's a pretty big oversight. It needs to be fixed before we merge.

More generally, we should try to expand the workflow lint tests to catch cases like this.

@Michael-T-McCann and @smajee : Could you look at the missing docstring issue?

Working on right now

Addressed in 079ac16

scico/linop/radon_svmbir.py

tbalke · 2021-11-15T22:47:46Z

scico/test/linop/test_radon_svmbir.py

@@ -40,6 +42,24 @@ def make_A(im, num_angles, num_channels):
    return A


+def cg_prox(f, v, λ):


Addressed in 72c77f8. Note slightly different way of handling the prox_kwargs because of changing variable names.

tbalke · 2021-11-15T22:57:18Z

scico/linop/radon_svmbir.py

+            max_iterations=self.prox_kwargs.get("maxiter"),
+            stop_threshold=self.prox_kwargs.get("ctol"),


When the prox_kwargs dictionary entries are None, this will cause it to use the svmbir defaults.

To get the scico defaults, need to set prox_kwargs={"maxiter": 1000, "ctol": 0.001} or not set prox_kwargs.

This may be a little counterintuitive. @bwohlberg ?

Agreed. Any suggestions?

This has been resolved by adjusting our expectations of what it means to override or omit entries in the passed dictionary.

The dictionary is seen as a whole and thus omitting an entry is like saying use whatever default the underlying method has.

Clarified this in 079ac16

bwohlberg · 2021-11-16T14:29:55Z

examples/scripts/ct_svmbir_ppp_bm3d_admm_prox.py

@@ -91,7 +91,9 @@
 ρ = 10  # ADMM penalty parameter
 σ = density * 0.26  # denoiser sigma

-f = SVMBIRWeightedSquaredL2Loss(y=y, A=A, W=Diagonal(weights), scale=0.5)
+f = SVMBIRWeightedSquaredL2Loss(
+    y=y, A=A, W=Diagonal(weights), scale=0.5, prox_kwargs={"maxiter": 5}


Is it really desirable that we just drop the default stopping tolerance here?

I think I would choose the tolerance to be 0 (or very) low so that always 5 iterations are being performed (unless there is really almost no change)

Addressed in a640fa7

bwohlberg · 2021-11-16T14:35:46Z

scico/linop/radon_svmbir.py

+    ``is_masked`` option defines a valid region for projection and updates.
+    Pixels outside the valid region are not projected or updated during
+    a reconstruction.
+    """


It's not clear what updates and reconstruction here refer too, since this class is just a Radon transform. Presumably it refers to the when the ParallelBeamProjector is used with SVMBIRWeightedSquaredL2Loss? If so, this should be clarified here.

Clarified in commit 219e5ed

Michael-T-McCann and others added 9 commits November 3, 2021 08:23

Rough in approximate prox function

717e1c6

Add kwargs to all proxes

9a5cfa6

change svmbir roi radius to have the full rectangle as the imaging do…

123d296

…main

update gitignore to ignore macos hidden files

f8d2cfb

pass initial image to prox

66fa592

add stopping conditions to SVMBIRWeightedSquaredL2Loss

5118df1

add optional is_masked argument to projector. SVMBIRWeightedSquaredL2…

08c5ba7

…Loss uses this argument as well.

Use bug-fixed branch of svmbir

812d3dc

use small number of iterations for proximal map in example

9e0ae41

smajee requested review from tbalke, Michael-T-McCann and bwohlberg and removed request for tbalke and Michael-T-McCann November 12, 2021 21:11

smajee added bug Something isn't working priority: high High priority labels Nov 12, 2021

smajee requested a review from Michael-T-McCann November 12, 2021 21:12

smajee linked an issue Nov 12, 2021 that may be closed by this pull request

Additional tests for svmbir interface #57

Closed

Merge branch 'main' into mike/approx_prox

0bcc1b3

smajee added 2 commits November 12, 2021 14:28

change CT angle generation so that pi is not included

3a6d97e

Merge branch 'mike/approx_prox' of github.com:lanl/scico into mike/ap…

0cb6003

…prox_prox

smajee removed a link to an issue Nov 12, 2021

Additional tests for svmbir interface #57

Closed

bwohlberg and others added 5 commits November 12, 2021 15:24

Merge branch 'main' into mike/approx_prox

8313a04

Apply black

7640be4

Merge branch 'main' into mike/approx_prox

56189bd

fix merge conflicts

1220f75

Fix lint error

93e03ac

smajee and others added 3 commits November 15, 2021 12:11

update docstring for svmbir projector

b342a7e

cast to Device

497e1ca

Merge branch 'mike/approx_prox' of github.com:lanl/scico into mike/ap…

d6c6d85

…prox_prox

tbalke mentioned this pull request Nov 15, 2021

Output types of operators #93

Closed

tbalke and others added 4 commits November 15, 2021 15:41

prox_kwargs

72c77f8

add generic prox for unweighted loss

9e45bdc

Update error messages

f4f268d

handle prox_args=None

57a06f2

This was linked to issues Nov 16, 2021

Example script ct_svmbir_ppp_bm3d_admm_cg.py broken #90

Closed

Additional tests for svmbir interface #57

Closed

tbalke added 2 commits November 15, 2021 17:17

deal better with non-set dictionary entries

f318cd1

remove testing file

57e0df0

add test for non-diagonal prox

53713dd

tbalke marked this pull request as ready for review November 16, 2021 00:55

tbalke approved these changes Nov 16, 2021

View reviewed changes

Some docstring improvements

0f5175f

bwohlberg reviewed Nov 16, 2021

View reviewed changes

tbalke and others added 7 commits November 16, 2021 08:19

add docstring

079ac16

added ctol

a640fa7

Merge branch 'main' into mike/approx_prox

db28d3f

update ParallelBeamProjector docstring

219e5ed

Some docstring improvements

4c02266

Improve parameters

9b614bb

Improve parameters

2cc51b3

bwohlberg approved these changes Nov 16, 2021

View reviewed changes

bwohlberg merged commit e87cda1 into main Nov 16, 2021

bwohlberg deleted the mike/approx_prox branch November 16, 2021 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to admm and svmbir wrapper to address issue #57 #88

Changes to admm and svmbir wrapper to address issue #57 #88

smajee commented Nov 12, 2021

bwohlberg commented Nov 12, 2021

smajee commented Nov 12, 2021

bwohlberg commented Nov 16, 2021

tbalke commented Nov 16, 2021

tbalke Nov 15, 2021

bwohlberg Nov 16, 2021

bwohlberg Nov 16, 2021

tbalke Nov 16, 2021

tbalke Nov 16, 2021

tbalke Nov 15, 2021

tbalke Nov 15, 2021

bwohlberg Nov 16, 2021

tbalke Nov 16, 2021 •

edited

tbalke Nov 16, 2021

bwohlberg Nov 16, 2021

tbalke Nov 16, 2021

tbalke Nov 16, 2021 •

edited

bwohlberg Nov 16, 2021

smajee Nov 16, 2021

		@@ -111,7 +143,9 @@ def _bproj_hcb(self, y):


		class SVMBIRWeightedSquaredL2Loss(WeightedSquaredL2Loss):

		@@ -40,6 +42,24 @@ def make_A(im, num_angles, num_channels):
		return A


		def cg_prox(f, v, λ):

		max_iterations=self.prox_kwargs.get("maxiter"),
		stop_threshold=self.prox_kwargs.get("ctol"),

Changes to admm and svmbir wrapper to address issue #57 #88

Changes to admm and svmbir wrapper to address issue #57 #88

Conversation

smajee commented Nov 12, 2021

bwohlberg commented Nov 12, 2021

smajee commented Nov 12, 2021

bwohlberg commented Nov 16, 2021

tbalke commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbalke Nov 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbalke Nov 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbalke Nov 16, 2021 •

edited

tbalke Nov 16, 2021 •

edited