merge

theano/compile/function_module.py

@@ -751,13 +751,19 @@ class FunctionMaker(object):
         if not isinstance(inputs, (list, tuple)):
             inputs = [inputs]
 
+
         # Wrap them in In or Out instances if needed.
+        #import pudb; pudb.set_trace()
         inputs, outputs =  map(self.wrap_in, inputs), map(self.wrap_out, outputs)
-        _inputs = gof.graph.inputs([o.variable for o in outputs] + [i.update for i in inputs if getattr(i, 'update', False)])
+        _inputs = gof.graph.inputs([o.variable for o in outputs] + [i.update 
+            for i in inputs if getattr(i, 'update', False)])
+
+        #TODO: REMOVE THIS CRUFT - it's complicated for SymbolicInputKits
         indices = [[input] + self.expand_in(input, _inputs) for input in inputs]
         expanded_inputs = reduce(list.__add__, [list(z) for x, y, z in indices], [])
+        assert expanded_inputs == inputs  #JB - I added this to make sure we could delete above
 
-        # make the env
+        # make the env (copies the graph, creates NEW INPUT AND OUTPUT VARIABLES)
         env, additional_outputs = std_env(expanded_inputs, outputs, accept_inplace)
         self.env = env
 
@@ -774,12 +780,34 @@ class FunctionMaker(object):
         # but some of the outputs can be shared variables, and is not good for shared
         # variables to be aliased. It might be possible to optimize this by making sure
         # there is no aliasing only between shared variables.
+
+        assert len(inputs) == len(env.inputs)
+
+        updated_env_inputs = [env_i for i, env_i in zip(inputs, env.inputs) if getattr(i, 'update', False)]
+
         for i in xrange(len(env.outputs)):
-            views = set()
-            view_tree_set(alias_root(env.outputs[i]), views)
+            views_of_output_i = set()
+            view_tree_set(alias_root(env.outputs[i]), views_of_output_i)
+            copied = False
+            # do not allow outputs to be aliased
             for j in xrange(i+1, len(env.outputs)):
-                if env.outputs[j] in views:
-                    env.change_input('output', j, deep_copy_op(env.outputs[j]))
+                if env.outputs[j] in views_of_output_i:
+                    env.change_input('output', i, deep_copy_op(env.outputs[i]))
+                    copied = True
+                    break
+
+            if not copied:
+                for input_j in env.inputs:
+                    # do not allow outputs to be aliased to an inputs (j), unless 
+                    # a) that j'th input has been 'destroyed' by e.g. in-place computations
+                    # b) that j'th input is a shared variable that is also being updated
+                    if hasattr(env,'get_destroyers_of') and env.get_destroyers_of(input_j):
+                        continue
+                    if input_j in updated_env_inputs:
+                        continue
+                    if input_j in views_of_output_i:
+                        env.change_input('output', i, deep_copy_op(env.outputs[i]))
+                        break
 
 
 

theano/compile/sharedvalue.py

@@ -64,11 +64,37 @@ class SharedVariable(Variable):
                     readonly=False,
                     strict=strict)
 
-    def __set(self,new_value):
-        self.container.value = new_value
+    def get_value(self, borrow=False):
+        """Get the non-symbolic value associated with this SharedVariable.
+
+        :param borrow: 
+            True to return the internal value directly, potentially creating problems related
+            to aliased memory.
+
+        If the return value is mutable, and you have used borrow=True to get at the internal
+        value, then you should be careful about changing it.  If you modify it, call
+        set_value(rval, borrow=True) to tell Theano that you modified it.  (Theano may have
+        cached computations based on the old value.)
+        
+        """
+        if borrow:
+            return self.container.value
+        else:
+            return copy.deepcopy(self.container.value)
 
-    def __get(self):
-        return self.container.value
+    def set_value(self,new_value, borrow=False):
+        """Set the non-symbolic value associated with this SharedVariable.
+
+        :param borrow: 
+            True to use the new_value directly, potentially creating problems
+            related to aliased memory.
+        
+        Changes to this value will be visible to all functions using this SharedVariable.
+        """
+        if borrow:
+            self.container.value = new_value
+        else:
+            self.container.value = copy.deepcopy(new_value)
 
     def clone(self):
         cp = self.__class__(
@@ -80,16 +106,9 @@ class SharedVariable(Variable):
         cp.tag = copy.copy(self.tag)
         return cp
 
-    value = property(__get, __set)
-    #value = self.container.value #GD- would've thought mapping one property to another would work
-
-    """Read/write the non-symbolic value associated with this SharedVariable.
-    
-    If the SharedVariable is shared, changes to this value will be visible to all functions using
-    this SharedVariable.  If this SharedVariable is not shared, a change will not be visible to
-    functions that were created before the change.
+    value = property(get_value, set_value, 
+            doc="shortcut for self.get_value() and self.set_value() which COPIES data")
 
-    """
 
     def filter_update(self, update):
         """When this shared variable is updated by a pfunc, the update value will be run through this function.

theano/compile/tests/test_function_module.py

@@ -285,7 +285,7 @@ class T_function(unittest.TestCase):
         a = T.dmatrix()
         f = function([a], Out(a, borrow=False))
         o = N.ones((3,3))
-        assert o is f(o) #borrow does not imply copy. 
+        assert o is not f(o) #function no longer permits aliasing outputs to inputs
 
         f = function([a], Out(a*4, borrow=False))
         o = N.ones((3,3))

theano/compile/tests/test_pfunc.py

@@ -8,6 +8,12 @@ from theano import tensor
 from theano.compile.sharedvalue import *
 from theano.compile.pfunc import *
 
+
+def data_of(s):
+    """Return the raw value of a shared variable"""
+    return s.container.storage[0]
+
+
 class Test_pfunc(unittest.TestCase):
 
     def test_doc(self):
@@ -135,9 +141,11 @@ class Test_pfunc(unittest.TestCase):
     def test_shared_mutable(self):
         bval = numpy.arange(5)
         b = shared(bval)
-        assert b.value is bval
         b_out = b * 2
 
+        assert b.value is not bval # shared vars copy args.
+        bval = data_of(b)          # so we do this to get at the underlying data
+
         # by default, shared are not mutable unless doing an explicit update
         f = pfunc([], [b_out], mode='FAST_RUN')
         assert (f() ==  numpy.arange(5) * 2).all()
@@ -152,6 +160,7 @@ class Test_pfunc(unittest.TestCase):
         # do not depend on updates being in-place though!
         bval = numpy.arange(5)
         b.value = bval
+        bval = data_of(b)
         f = pfunc([], [b_out], updates=[(b, b_out+3)], mode='FAST_RUN')
         assert ( f() == numpy.arange(5)*2 ).all()
         assert (b.value == ((numpy.arange(5)*2)+3)).all() # because of the update
@@ -169,17 +178,6 @@ class Test_pfunc(unittest.TestCase):
         assign()
         self.failUnless(x.value == 3)
 
-        # Same but using a mutable constant to show how it can be used to
-        # modify the update value after the function is created.
-        x.value = 0
-        y = numpy.ones((), dtype='int64')
-        assign_mutable = pfunc([], [], updates = {x: y})
-        assign_mutable()
-        self.failUnless(x.value == 1)
-        y.fill(4)
-        assign_mutable()
-        self.failUnless(x.value == 4)
-
         # Basic increment function.
         x.value = 0
         inc = pfunc([], [], updates = {x: x + 1})
@@ -474,10 +472,52 @@ class Test_pfunc(unittest.TestCase):
         assert f() == 21
         assert f() == 34
 
-    def aliasing_test(self):
 
-        A = shared(numpy.zeros((2,2)))
-        B = shared(numpy.zeros((2,2)))
+
+
+
+
+class Test_aliasing_rules(unittest.TestCase):
+    """
+    1. Theano manages its own memory space, which typically does not overlap with the memory of
+    normal python variables that the user uses.
+
+    2. shared variables are allocated in this memory space, as are the temporaries used for
+    Function evalution.
+
+    3. Physically, this managed memory space may be spread across the host, on a GPU device(s),
+    or even on a remote machine.
+
+    4. Theano assumes that shared variables are never aliased to one another, and tries to make
+    it impossible to accidentally alias them.
+
+    5. Theano's managed data is constant while Theano Functions are not running and theano
+    library code is not running.
+
+    6. The default behaviour of Function is to return user-space values for outputs, but this
+    can be overridden (borrow=True) for better performance, in which case the returned value
+    may be aliased to managed memory, and potentially invalidated by the next Theano Function
+    call or call to theano library code.
+    """
+
+    def shared(self, x):
+        return tensor.shared(x)
+
+    def test_shared_constructor_copies(self):
+        # shared constructor makes copy
+        # (rule #2)
+        orig_a = numpy.zeros((2,2))
+        A = self.shared(orig_a)
+        assert not numpy.may_share_memory(orig_a, data_of(A))
+
+        # rule #2 reading back from theano-managed memory
+        assert not numpy.may_share_memory(A.value, data_of(A))
+        
+
+    def test_potential_output_aliasing_induced_by_updates(self):
+
+        A = self.shared(numpy.zeros((2,2)))
+        B = self.shared(numpy.zeros((2,2)))
         C = numpy.zeros((2,2))
         D = tensor.dmatrix()
         DD = D + 5
@@ -485,41 +525,141 @@ class Test_pfunc(unittest.TestCase):
         f = pfunc([D], [], updates=[ (A,D), (B,D) ])
         f(C)
 
-        assert not numpy.may_share_memory(A.value,B.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
         f = pfunc([D], [], updates=[ (A,D[:]), (B,D) ])
         f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
         f = pfunc([D], [], updates=[ (A,D+5), (B,D[:]) ])
         f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
 
         f = pfunc([D], [], updates=[ (A,D+5), (B,D) ])
         f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
 
         f = pfunc([D], DD, updates=[ (A,DD[:1]), (B,DD) ])
         R=f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
-        assert not numpy.may_share_memory(R,B.value)
-        assert not numpy.may_share_memory(R,A.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
+        assert not numpy.may_share_memory(R,data_of(B))
+        assert not numpy.may_share_memory(R,data_of(A))
 
         f = pfunc([D], DD, updates=[ (A,DD[:1]), (B,DD[:1]*2) ])
         R=f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
-        assert not numpy.may_share_memory(R,B.value)
-        assert not numpy.may_share_memory(R,A.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
+        assert not numpy.may_share_memory(R,data_of(B))
+        assert not numpy.may_share_memory(R,data_of(A))
 
         f = pfunc([D], DD*4, updates=[ (A,DD[:1]*3), (B,DD[:1]*2) ])
         R=f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
-        assert not numpy.may_share_memory(R,B.value)
-        assert not numpy.may_share_memory(R,A.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
+        assert not numpy.may_share_memory(R,data_of(B))
+        assert not numpy.may_share_memory(R,data_of(A))
 
         f = pfunc([D], DD*4, updates=[ (A,DD[:1]*3), (B,DD[:1]*3) ])
         R=f(C)
-        assert not numpy.may_share_memory(A.value,B.value)
-        assert not numpy.may_share_memory(R,B.value)
-        assert not numpy.may_share_memory(R,A.value)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
+        assert not numpy.may_share_memory(R,data_of(B))
+        assert not numpy.may_share_memory(R,data_of(A))
+
+    def test_no_aliasing_0(self):
+        # B is a shared variable, A is updated with B's contents
+        # we need A to be copied to avoid aliasing
+        A = self.shared(numpy.zeros((2,2))+.5)
+        B = self.shared(numpy.zeros((2,2))-.5)
+        f = pfunc([], [], updates=[(A,B)])
+        f()
+        assert not numpy.may_share_memory(data_of(A), data_of(B))
+
+    def test_no_aliasing_1(self):
+        # B is a shared variable, A is updated with B's contents
+        # since B is being updated as well, we don't need to copy anything to avoid aliasing
+        # shared variables.
+        A = self.shared(numpy.zeros((2,2))+.5)
+        B = self.shared(numpy.zeros((2,2))-.5)
+        C = tensor.dmatrix()
+        f = pfunc([C], [], updates=[ (A,B), (B,C) ])
+        z = numpy.zeros((2,2))
+        f(z)
+        assert not numpy.may_share_memory(data_of(A),data_of(B))
+        assert not numpy.may_share_memory(z,data_of(B)) # Theano tries to maintain its own memory space.
+        assert numpy.all(data_of(B) == z)
+
+    def test_no_aliasing_2(self):
+        # B and A take one another's values
+        # no copying is necessary since each one is updated.
+        orig_a = numpy.zeros((2,2))+.5
+        orig_b = numpy.zeros((2,2))-.5
+        A = self.shared(orig_a)
+        B = self.shared(orig_b)
+        C = tensor.dmatrix()
+
+        z = numpy.zeros((2,2))
+
+        data_of_a = data_of(A)
+        data_of_b = data_of(B)
+
+        f = pfunc([C], [], updates=[(A,B),(B,A)])
+        f(z)
+        # correctness
+        assert numpy.all(data_of(A) == -.5)
+        assert numpy.all(data_of(B) == +.5)
+
+        # shared vars may not be aliased
+        assert not numpy.may_share_memory(data_of(A), data_of(B))
+
+        # theano should have been smart enough to not make copies
+        assert numpy.may_share_memory(data_of(A), data_of_b)
+        assert numpy.may_share_memory(data_of(B), data_of_a)
+
+    def test_no_aliasing_2b(self):
+        # B and A take one another's values
+        # no copying is necessary since each one is updated.
+        # The twist one `test_no_aliasing_2` is that each shared var is updated with a view of
+        # the other one.
+
+        orig_a = numpy.zeros((2,2))+.5
+        orig_b = numpy.zeros((2,2))-.5
+        A = self.shared(orig_a)
+        B = self.shared(orig_b)
+        C = tensor.dmatrix()
+
+        z = numpy.zeros((2,2))
+
+        data_of_a = data_of(A)
+        data_of_b = data_of(B)
+
+        f = pfunc([C], [], updates=[(A,B[:,::-1]),(B,A.T)])
+        theano.printing.debugprint(f)
+        f(z)
+        # correctness (doesn't actually test the view...)
+        assert numpy.all(data_of(A) == -.5)
+        assert numpy.all(data_of(B) == +.5)
+
+        # shared vars may not be aliased
+        assert not numpy.may_share_memory(data_of(A), data_of(B))
+
+        # theano should have been smart enough to not make copies
+        assert numpy.all(data_of(A) < 5)
+        data_of_b += 10
+        assert numpy.all(data_of(A) > 5)
+        data_of_b -= 10
+
+        assert numpy.all(data_of(B) < 5)
+        data_of_a += 10
+        print data_of(B)
+        assert numpy.all(data_of(B) > 5)
+        data_of_a -= 10
+
+        # N.B. may_share_memory is what we mean, but does it work?
+        assert numpy.may_share_memory(data_of(A), data_of_b)
+        assert numpy.may_share_memory(data_of(B), data_of_a)
+
+        # N.B. This pattern could form a memory leak - each shared variable always points to a
+        # view, and that view gets further and further from the (e.g. data_of_a) with each
+        # call.  The memory leak is in the increasing number of view objects forming a chain to
+        # the underlying data.
+
+
 
 if __name__ == '__main__':
     theano.config.mode = 'FAST_COMPILE'

theano/compile/tests/test_shared.py

@@ -107,8 +107,8 @@ class Test_SharedVariable(unittest.TestCase):
 
         # check that an assignment of a perfect value results in no copying
         uval = theano._asarray([5,6,7,8], dtype='float64')
-        u.value = uval
-        assert u.value is uval
+        u.set_value(uval, borrow=True)
+        assert u.get_value(borrow=True) is uval
 
     def test_scalar_strict(self):
         def f(var, val): var.value = val

theano/gradient.py

@@ -32,6 +32,8 @@ def grad_sources_inputs(sources, graph_inputs, warn_type=True):
     """
     gmap = {}
     for (r, g_r) in sources:
+        if not hasattr(r, 'type'):
+            raise TypeError('sources must be Variables', r)
         if g_r is not None:
             if r in gmap:
                 gmap[r] = gmap[r] + g_r
@@ -52,6 +54,10 @@ def grad_sources_inputs(sources, graph_inputs, warn_type=True):
         output_arg = g_outputs
         input_arg = node.inputs
 
+        # Each Op's grad function requires inputs and output_grads
+        # If the Op destroys any input, but the grad expression uses it, then chances are the
+        # resulting graph will have a dependency cycle.  We avoid this cycle by passing
+        # (symbolic) copies of each destroyed input.
         try:
             dinputs = [node.inputs[x[0]] for x in node.op.destroy_map.values()]
         except AttributeError:
@@ -93,6 +99,7 @@ def grad_sources_inputs(sources, graph_inputs, warn_type=True):
             if g_r and len(sources) == 1 and sources[0][0].name and r.name:
                 g_r.name = "(d%s/d%s)" % (sources[0][0].name, r.name)
             if g_r is not None: 
+                assert r is not None
                 if r in gmap:
                     gmap[r] = gmap[r] + g_r
                 else:

theano/tensor/sharedvar.py

@@ -27,7 +27,7 @@ def tensor_constructor(value, name=None, strict=False, broadcastable=None):
     if broadcastable is None:
         broadcastable = (False,)*len(value.shape)
     type = TensorType(value.dtype, broadcastable=broadcastable)
-    return TensorSharedVariable(type=type, value=value, name=name, strict=strict)
+    return TensorSharedVariable(type=type, value=numpy.array(value,copy=True), name=name, strict=strict)
 
 # TensorSharedVariable brings in the tensor operators, is not ideal, but works as long as we
 # dont do purely scalar-scalar operations 
@@ -56,7 +56,7 @@ def scalar_constructor(value, name=None, strict=False):
         # Do not pass the dtype to asarray because we want this to fail if
         # strict is True and the types do not match.
         rval = ScalarSharedVariable(type=tensor_type,
-                value=numpy.asarray(value),
+                value=numpy.array(value, copy=True),
                 name=name, strict=strict)
         return rval
     except: