Add as_cvx_operator and example #494

adler-j · 2016-07-14T12:42:40Z

Preliminary work on adding a CVX interface to ODL.

The backend is now available here: proximal-lang.org and on github: ProxImaL

Todo:

Decide on style. Is this good?
Naming. This does not actually use CVX, but uses "proximal", which happens to be a not that good name for us. What should we name this?
Add proximal as an optional dependency to ODL.
Add documentation on the existence of this interface, both to README.md and to the full doc.

adler-j · 2016-07-14T13:45:39Z

examples/solvers/cvx_solvers.py

+
+    -Laplacian(x) = b
+
+where b is a gaussian peak at the origin.


adler-j · 2016-07-26T15:28:32Z

Code now works properly and gives a decent result. Ready for review.

adler-j · 2016-07-26T16:24:02Z

odl/operator/oputils.py

+    """
+
+    def forward(inp, out):
+        out[:] = op(inp).asarray()


Use the out parameter

adler-j · 2016-07-26T18:54:06Z

examples/solvers/cvx_solvers_tomography.py

+
+# --- Set up the inverse problem --- #
+
+# Note that proximal is not aware of the problem scaling


Needs to be clarified. What I'm aiming at is that proximal is fully discrete and does not know of scalings for the norms etc, hence this may be missleading since users would assume grad is the same as the gradient in ODL, etc.

The gradient is a linear operator, right? So would it be possible to use the gradient from ODL by wrapping it in the same way as ray_transform -> cvx_ray_transform?

Sure, but then we would need to add asarray and shape to ProductSpaces, which is perhaps about time.

Even after that, the norm operators would still differ.

That's true... so if it cannot be fixed maybe a clarifying comment about it is enough

ozanoktem · 2016-07-28T09:14:45Z

A comment regarding naming: The term proximal is, as already mentioned by @adler-j, not suitable since we use it for other purposes. Since Python is case sensitive, we could perhaps consider using ProxImaL? Another alternative is CVXproximal.

aringh · 2016-07-28T09:36:19Z

I think it looks very good.

Regarding the naming: I'm not sure I like ProxImal. It still feels to easily confused with what we call proximal operators, if we have a method called as_ProxImal_operator. Then I think as_cvx_proximal_operator or something like that would be better (although it is not technically cvx. However, I guess the basic idea was first implemented here https://github.com/mfopt/mf_cvxpy, which is still called cvx)

adler-j · 2016-07-31T06:45:26Z

I'll actually invite @SteveDiamond to please give some input here. What would you like the function currently called as_cvx_operator to be named? As mentioned above, the word "proximal" is already taken in ODL, so the obvious as_proximal_operator cannot be done.

SteveDiamond · 2016-07-31T23:36:34Z

How about as_prox_lang_operator? We call it the ProxImaL modeling language.

adler-j · 2016-08-01T07:03:09Z

Good suggestion, I like it. Do we agree?

ozanoktem · 2016-08-01T07:07:13Z

I think as_prox_lang_operator, as suggested by @SteveDiamond, is ok but not ideal. Since I cannot come up with a better alternative, I support using this suggestion.

aringh · 2016-08-02T07:14:17Z

I think it sounds like a good name.

kohr-h · 2016-08-09T10:23:42Z

I think the code is nice and simple, a good sign that ODL is extensible and does not lock in anybody. And I'm glad to see progress in the integration with ProxImaL.

Todo:

Decide on style. Is this good?

What alternatives do you have in mind? If you mean the interface, it's very similar to the SciPy linear operator interface, so we're consistent, which is good.

Add proximal as an optional dependency to ODL.

OK.

How about as_prox_lang_operator? We call it the ProxImaL modeling language.

Sounds good to me.

A comment regarding naming: The term proximal is, as already mentioned by @adler-j, not suitable since we use it for other purposes. Since Python is case sensitive, we could perhaps consider using ProxImaL? Another alternative is CVXproximal.

I guess you mean the import proximal statement in the beginning? I agree that this can lead to confusion with our internal proximal sub-package. Since we seem to have settled with prox_lang as infix in the function name, why don't we avoid the conflict by import proximal as prox_lang?

kohr-h · 2016-08-15T14:44:07Z

examples/solvers/proximal_lang_tomography.py

+# The implementation of the ray transform to use, options:
+# 'scikit'                    Requires scikit-image (can be installed by
+#                             running ``pip install scikit-image``).
+# 'astra_cpu', 'astra_cuda'   Require astra tomography to be installed.


kohr-h · 2016-08-15T15:00:25Z

Since this is 80 % examples, they should be clear and have good comments. Therefore I was a bit picky there. Interface looks fine to me, and the selection of examples is also good.

adler-j · 2016-08-17T08:14:53Z

doc/source/release_notes.rst

@@ -11,8 +11,7 @@ Next release

 New features
 ------------
- First implementation of a deformation model based on linearized deformations using displacement


This was moved to a 0.4.0 release in another PR.

adler-j · 2016-08-17T08:15:09Z

Fixed all comments. Merge?

ozanoktem · 2016-08-17T08:19:53Z

Yes, go ahead and merge :-)

kohr-h · 2016-08-17T08:47:06Z

doc/source/guide/in_depth/proximal_lang_guide.rst

+   >>> 1 / np.sqrt(3)  # exact result
+   0.577350269189
+
+this is not the case in proximal, where the norm depends on the number of discretization points. Hence a scaling that is correct for a problem in ODL needs not be correct in proximal. This also changes the definition of things like the operator norm.


Can you change all occurrences of "proximal" to "ProxImaL" where applicable?

kohr-h · 2016-08-17T08:50:55Z

Some minor points, good for a merge after that.

kohr-h · 2016-08-17T09:24:39Z

doc/source/guide/glossary.rst

                \text{prox}_f(v) = \arg\min_x f(x) + (1/2)||x - v||_2^2
-
+
+        proximal is also occationally used instead of ProxImaL, then refering to the proximal


occasionally ("s", not "t")

adler-j added the type: deficiency label Jul 14, 2016

adler-j force-pushed the cvx_interface branch 3 times, most recently from ee390bf to 8baecd3 Compare July 14, 2016 13:28

adler-j reviewed Jul 14, 2016
View reviewed changes

examples/solvers/cvx_solvers.py

-Laplacian(x) = b

where b is a gaussian peak at the origin.

Copy link

Member Author

adler-j Jul 14, 2016

Choose a reason for hiding this comment

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

wrong

adler-j added status: review needed and removed type: deficiency labels Jul 26, 2016

adler-j reviewed Jul 26, 2016
View reviewed changes

odl/operator/oputils.py

"""

def forward(inp, out):

out[:] = op(inp).asarray()

Copy link

Member Author

adler-j Jul 26, 2016

Choose a reason for hiding this comment

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

Use the out parameter

adler-j force-pushed the cvx_interface branch from 8baecd3 to a14ccca Compare July 26, 2016 18:45

adler-j reviewed Jul 26, 2016
View reviewed changes

adler-j mentioned this pull request Aug 4, 2016

Example of using patch_NLM with a solver comp-imaging/ProxImaL#10

Closed

kohr-h reviewed Aug 15, 2016
View reviewed changes

adler-j force-pushed the cvx_interface branch from e8858a3 to b27b7eb Compare August 17, 2016 07:40

adler-j added 3 commits August 17, 2016 10:00

Add as_cvx_operator and example

1b38d23

DOC: improve doc of proximal interface

10c5185

DOC: doc updates to proximal language interface

3ad710f

adler-j force-pushed the cvx_interface branch from b27b7eb to 06e2d3f Compare August 17, 2016 08:14

DOC: add release info for proximal interface

06e2d3f

adler-j reviewed Aug 17, 2016
View reviewed changes

adler-j added merge? and removed status: review needed labels Aug 17, 2016

kohr-h reviewed Aug 17, 2016
View reviewed changes

DOC: rename proximal -> ProxImaL where appropriate

bd7da98

adler-j merged commit a0f723e into master Aug 17, 2016

adler-j deleted the cvx_interface branch August 17, 2016 09:01

kohr-h reviewed Aug 17, 2016
View reviewed changes

kohr-h removed the merge? label Sep 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add as_cvx_operator and example #494

Add as_cvx_operator and example #494

adler-j commented Jul 14, 2016 •

edited by kohr-h

Loading

adler-j Jul 14, 2016

adler-j commented Jul 26, 2016

adler-j Jul 26, 2016

adler-j Jul 26, 2016

aringh Jul 28, 2016

adler-j Jul 28, 2016

adler-j Jul 28, 2016

aringh Jul 28, 2016

ozanoktem commented Jul 28, 2016

aringh commented Jul 28, 2016

adler-j commented Jul 31, 2016

SteveDiamond commented Jul 31, 2016

adler-j commented Aug 1, 2016

ozanoktem commented Aug 1, 2016

aringh commented Aug 2, 2016

kohr-h commented Aug 9, 2016

kohr-h Aug 15, 2016

kohr-h commented Aug 15, 2016

adler-j Aug 17, 2016

adler-j commented Aug 17, 2016 •

edited

Loading

ozanoktem commented Aug 17, 2016

kohr-h Aug 17, 2016

kohr-h commented Aug 17, 2016

kohr-h Aug 17, 2016


		# --- Set up the inverse problem --- #

		# Note that proximal is not aware of the problem scaling

		\text{prox}_f(v) = \arg\min_x f(x) + (1/2)\|\|x - v\|\|_2^2


		proximal is also occationally used instead of ProxImaL, then refering to the proximal

Add as_cvx_operator and example #494

Add as_cvx_operator and example #494

Conversation

adler-j commented Jul 14, 2016 • edited by kohr-h Loading

Choose a reason for hiding this comment

adler-j commented Jul 26, 2016

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

ozanoktem commented Jul 28, 2016

aringh commented Jul 28, 2016

adler-j commented Jul 31, 2016

SteveDiamond commented Jul 31, 2016

adler-j commented Aug 1, 2016

ozanoktem commented Aug 1, 2016

aringh commented Aug 2, 2016

kohr-h commented Aug 9, 2016

Choose a reason for hiding this comment

kohr-h commented Aug 15, 2016

Choose a reason for hiding this comment

adler-j commented Aug 17, 2016 • edited Loading

ozanoktem commented Aug 17, 2016

Choose a reason for hiding this comment

kohr-h commented Aug 17, 2016

Choose a reason for hiding this comment

adler-j commented Jul 14, 2016 •

edited by kohr-h

Loading

adler-j commented Aug 17, 2016 •

edited

Loading