Problem with numpy arrays as step lengths to proximal operators?

[aringh]

While working on the Bregman distance functional in #1267 I encountered the following bug:

import odl
import numpy as np

space = odl.uniform_discr(0, 1, 10)
point = space.one()

l2_sq = odl.solvers.L2NormSquared(space)
breg = odl.solvers.BregmanDistance(l2_sq, point)

sigma_array = np.array(0.4)
sigma = float(sigma_array)

tmp = breg.proximal
tmp(sigma)
tmp(sigma_array)

which gives a very short call stack and a cryptical error message.

Traceback (most recent call last):

  File "<ipython-input-4-2907eb4487d1>", line 1, in <module>
    runfile('[...]/tmp.py', wdir='[...]')

  File "[...]/miniconda3/envs/odl-py36/lib/python3.6/site-packages/spyder/utils/site/sitecustomize.py", line 880, in runfile
    execfile(filename, namespace)

  File "[...]/miniconda3/envs/odl-py36/lib/python3.6/site-packages/spyder/utils/site/sitecustomize.py", line 102, in execfile
    exec(compile(f.read(), filename, 'exec'), namespace)

  File "[...]/tmp.py", line 15, in <module>
    tmp(sigma_array)

TypeError: unhashable type: 'numpy.ndarray'

Does anyone know what the problem is?

[sbanert]

The problem is that the argument of the proximal factory is cached in order to increase the performance by not creating copies of the same operator. This does not work, since numpy.ndarray is mutable and hence cannot be hashed. This behaviour is switched off once #1166 is merged. You could try replacing the following line by sig = float(self.sigma).

odl/odl/solvers/nonsmooth/proximal_operators.py

Line 926 in 3b75c07

sig = self.sigma

[kohr-h]

Well explained, @sbanert.

A general remark: This is one of the rare cases where we store user input as-is. It's easier for sure, since sanitizing code needs to be written and maintained. But the downside is that you never know for sure what you have stored. Still, you have a type in mind (here float) that you implicitly assume when writing depending code, and when that assumption is violated, bad things can happen.
So in general, whenever we store something as a class attribute, we try to always store a well-defined type or, limit the possible types to 2 or 3 (when sanitizing code would be expensive), or in the worst case at least write a comment that we need to treat the attribute as user-defined and cannot know for sure what it is (at least that's what we should do in that case 😄).

So the short version of this is: when storing user input, always sanitize.

[aringh]

Ok, thank you both @sbanert and @kohr-h for the explanation.

Regarding the suggested change: that is already happening in the init method, 5 lines above. That is why I did not understand what was happening. Exactly when is exactly what cashed and hashed?

[sbanert]

Yes, I overlooked this line, sorry for the mistake. The caching is done with the decorator cache_arguments:
https://github.com/odlgroup/odl/search?utf8=%E2%9C%93&q=cache_arguments&type=

I think the argument is cached when trying to call the proximal of the quadratic perturbation.

[adler-j]

Should this be fixed now on master @sbanert?

[sbanert]

Seems to be fixed now. @aringh’s code does no longer trigger an error when I rebase his branch on the current master.