API: Changed FunctionalLinearPertub to quadratic #1066

adler-j · 2017-07-07T22:41:16Z

I guess we could use some more tests but this is very close to done.

adler-j · 2017-07-07T22:42:13Z

odl/test/largescale/solvers/nonsmooth/default_functionals_slow_test.py

 dual = simple_fixture('dual', [False, True])


 func_params = ['l1', 'l2', 'l2^2', 'kl', 'kl_cross_ent', 'const',
               'groupl1-1', 'groupl1-2',
-               'nuclearnorm-1-1', 'nuclearnorm-1-2', 'nuclearnorm-1-inf'


That this bug has been there for quite some time is scary. How often do we run largescale tests?

adler-j · 2017-07-08T15:20:21Z

This should now be feature complete, since @matthiasje suggested this change, perhaps you could help out with a quick review, especially of the features?

adler-j · 2017-07-08T15:21:21Z

odl/solvers/functional/functional.py

+            return self.functional.convex_conj.translated(
+                self.linear_term)
+        else:
+            return super().convex_conj


I guess this should be changed to python 2 style super calls

Would be reasonable I think.

kohr-h

I have one general comment about the types of tests that I think we need to avoid, namely tests that more or less just repeat the implementation.

Other than that, it's just some doc things, otherwise good to go.

kohr-h · 2017-07-09T19:51:19Z

odl/solvers/functional/functional.py

        """Initialize a new instance.

        Parameters
        ----------
        func : `Functional`
            Function corresponding to ``f``.
-        linear_term : `domain` element
+        quadratic_term : ``domain.field`` element, optional


"Term" in what sense? A quadratic term should be quadratic, this is just a scalar constant.

Any better suggestion? In math I'd just call this something like a, but that clearly won't work here.

How about quadratic_coeff? It's a polynomial after all.

In some sense, the linear_term (name also not ideal) is also just a vector of coefficients, at least in the discrete case. Otherwise it's more of a factor than a term.

quadratic_coeff is a great idea! I'll give the linear one some thought.

kohr-h · 2017-07-09T19:52:53Z

odl/solvers/functional/functional.py


-    """The ``Functional`` representing ``f(.) + <linear_term, .>``."""
+    """The ``Functional`` representing ``F(.) + a * |.|^2 + <., u>``."""


I'd write replace ``Functional`` by `Functional` but these links don't work in the one-line summaries, so just go for "functional"

The norm could be anything, I'd prefer a * <., .> + <., u>.

kohr-h · 2017-07-09T19:56:11Z

odl/solvers/functional/functional.py

        """Initialize a new instance.

        Parameters
        ----------
        func : `Functional`
            Function corresponding to ``f``.
-        linear_term : `domain` element
+        quadratic_term : ``domain.field`` element, optional
+            Quadratic term.


Mention nonnegativity requirement.

Second thought, why do we need that. Its only a requirement for the proximal. I'll remove the requirement.

kohr-h · 2017-07-09T19:57:30Z

odl/solvers/functional/functional.py

+        if linear_term is not None:
+            self.__linear_term = func.domain.element(linear_term)
+        else:
+            self.__linear_term = func.domain.zero()


This seems like a waste of space for questionable gain in _call.
And it won't work for domains that are not linear spaces.

Main idea here is that users should be able to simply call FunctionalQuadraticPerturb(func, quadratic_term=3.2) without mentioning the linear term.

Edit:

I see that you want me to move the logic into _call, adjoint, etc. I'll probably leave that out for now untill we see if anyone uses this function enough that such stuff will be noticeable for performance.

Fair enough, but there's still the issue that the functional doesn't work with e.g. RealNumbers like that.

RealNumbers does not impement inner, currently at least, so that ain't much of an issue for now.

But still weird to let this guy raise an error for the default in that case.

kohr-h · 2017-07-09T20:00:24Z

odl/solvers/functional/functional.py

+            self.__linear_term = func.domain.element(linear_term)
+        else:
+            self.__linear_term = func.domain.zero()
+
        # Only compute the grad_lipschitz if it is not inf
        if (not func.grad_lipschitz == np.inf and


if func.grad_lipschitz != np.inf

kohr-h · 2017-07-09T20:01:39Z

odl/solvers/functional/functional.py


    @property
    def functional(self):
        """The original functional."""
        return self.__functional

    @property
+    def quadratic_term(self):
+        """The translation."""


kohr-h · 2017-07-09T20:04:06Z

odl/solvers/functional/functional.py

+            return self.functional.convex_conj.translated(
+                self.linear_term)
+        else:
+            return super().convex_conj


Would be reasonable I think.

kohr-h · 2017-07-09T20:06:55Z

odl/solvers/nonsmooth/proximal_operators.py


    .. math::
-        \mathrm{prox}_{\\sigma F( \cdot scal)}(x) =
-        \\frac{1}{scal} \mathrm{prox}_{\\sigma \, scal^2 \, F }(x \, scal)
+        \mathrm{prox}_{\\sigma F( \cdot \\alpha)}(x) =


\\alpha \cdot

kohr-h · 2017-07-09T20:07:21Z

odl/solvers/nonsmooth/proximal_operators.py

-        \\frac{1}{scal} \mathrm{prox}_{\\sigma \, scal^2 \, F }(x \, scal)
+        \mathrm{prox}_{\\sigma F( \cdot \\alpha)}(x) =
+        \\frac{1}{\\alpha}
+        \mathrm{prox}_{\\sigma \, \\alpha^2 \, F }(x \, \\alpha)


\\alpha x

kohr-h · 2017-07-09T21:19:05Z

odl/test/solvers/functional/functional_test.py

+    assert all_almost_equal(functional(x),
+                            (orig_func(x) +
+                             quadratic_term * x.inner(x) +
+                             x.inner(linear_term)),


I'm honestly not so sure about these kinds of tests. What I mean by "these tests" is tests of the implementation of something rather than the intended functionality of something.

What I think one should do instead (and this is not easy while implementing stuff) is to take a step back, ask oneself "What is this function supposed to do?" and then test that. Or test things that implicitly follow but aren't directly implemented.
In this case it's almost the same, but there are still some differences, for instance what happens with weighted spaces. Errors there won't be caught by this test.

Here I think it's better to test an explicit example, and when we feel we need to settle on some behavior regarding weighting (it will influence the gradient) we add a test for that.

I'm honestly not so sure about these kinds of tests. What I mean by "these tests" is tests of the implementation of something rather than the intended functionality of something.

Well (imo) these dont test anything right now, but are only there as regression tests to give us a huge red flag if something gets changed so we actually notice. I don't really see any issue with that. They also make sure the code is run at least once which stops the worst of bugs.

With that said, you are certainly correct in that this could be done in a better manner, but that would require quite some effort here, something I probably wouldn't feel is warranted for a function that is as specialized as this once.

In other places, like Gradient which is called all the time, I certainly agree and a proper look at those tests is sorely needed.

I agree that it would be quite a bit of work and maybe not something to do right now. When writing new tests, however, we should keep this in mind and write "proper" tests right from the beginning.

adler-j commented Jul 7, 2017

View reviewed changes

adler-j added the status: review needed label Jul 8, 2017

adler-j force-pushed the issue-1064__quadratic_pertubation branch from bef579d to ba792b9 Compare July 8, 2017 15:20

adler-j commented Jul 8, 2017

View reviewed changes

kohr-h approved these changes Jul 9, 2017

View reviewed changes

adler-j added 3 commits July 10, 2017 15:11

API: Changed FunctionalLinearPertub to quadratic, closes odlgroup#1064

097d7a2

TST: Update tests for quadratic pertubation

c1d41a1

MAINT: style improvements to functionals

d0c3aac

adler-j force-pushed the issue-1064__quadratic_pertubation branch from 37978c4 to d0c3aac Compare July 10, 2017 13:11

adler-j merged commit bab08e8 into odlgroup:master Jul 10, 2017

adler-j deleted the issue-1064__quadratic_pertubation branch July 10, 2017 13:28

kohr-h removed the status: review needed label Jul 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: Changed FunctionalLinearPertub to quadratic #1066

API: Changed FunctionalLinearPertub to quadratic #1066

adler-j commented Jul 7, 2017

adler-j Jul 7, 2017

adler-j commented Jul 8, 2017

adler-j Jul 8, 2017

kohr-h Jul 9, 2017

kohr-h left a comment

kohr-h Jul 9, 2017

adler-j Jul 9, 2017

kohr-h Jul 10, 2017

kohr-h Jul 10, 2017

adler-j Jul 10, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

adler-j Jul 10, 2017

kohr-h Jul 9, 2017

adler-j Jul 9, 2017 •

edited

Loading

kohr-h Jul 10, 2017

adler-j Jul 10, 2017

kohr-h Jul 10, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

kohr-h Jul 9, 2017

adler-j Jul 9, 2017

kohr-h Jul 10, 2017


		"""The ``Functional`` representing ``f(.) + <linear_term, .>``."""
		"""The ``Functional`` representing ``F(.) + a * \|.\|^2 + <., u>``."""

API: Changed FunctionalLinearPertub to quadratic #1066

API: Changed FunctionalLinearPertub to quadratic #1066

Conversation

adler-j commented Jul 7, 2017

Choose a reason for hiding this comment

adler-j commented Jul 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h left a 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

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

adler-j Jul 9, 2017 • edited Loading

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

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

adler-j Jul 9, 2017 •

edited

Loading