Remove bug in optimization. Add warning. Update test

doc/library/config.txt

@@ -526,9 +526,9 @@ import theano and print the config variable, as in:
 .. attribute:: config.warn.ignore_bug_before
 
     String value: ``'None'``, ``'all'``, ``'0.3'``, ``'0.4'``, ``'0.4.1'``, ``'0.5'``,
-    ``'0.6'``, ``'0.7'``, ``'0.8'``, ``'0.8.1'``, ``'0.8.2'``, ``'0.9'``
+    ``'0.6'``, ``'0.7'``, ``'0.8'``, ``'0.8.1'``, ``'0.8.2'``, ``'0.9'``, ``'0.10'``
 
-    Default: ``'0.7'``
+    Default: ``'0.8'``
 
     When we fix a Theano bug that generated bad results under some
     circumstances, we also make Theano raise a warning when it encounters

theano/configdefaults.py

@@ -653,12 +653,19 @@ AddConfigVar('warn.ignore_bug_before',
               "bugs found after that version. "
               "Warning for specific bugs can be configured with specific "
               "[warn] flags."),
-             EnumStr('0.7', 'None', 'all', '0.3', '0.4', '0.4.1', '0.5', '0.6',
-                     '0.7', '0.8', '0.8.1', '0.8.2', '0.9',
+             EnumStr('0.8', 'None', 'all', '0.3', '0.4', '0.4.1', '0.5', '0.6',
+                     '0.7', '0.8', '0.8.1', '0.8.2', '0.9', '0.10',
                      allow_override=False),
              in_c_key=False)
 
 
+def split_version(version):
+    """
+    Take version as a dot-separated string, return a tuple of int
+    """
+    return tuple(int(i) for i in version.split('.'))
+
+
 def warn_default(version):
     """
     Return True iff we should warn about bugs fixed after a given version.
@@ -667,7 +674,8 @@ def warn_default(version):
         return True
     if config.warn.ignore_bug_before == 'all':
         return False
-    if config.warn.ignore_bug_before >= version:
+    if (split_version(config.warn.ignore_bug_before) >=
+            split_version(version)):
         return False
     return True
 
@@ -759,11 +767,21 @@ AddConfigVar('warn.inc_set_subtensor1',
              in_c_key=False)
 
 AddConfigVar('warn.round',
-             "Round changed its default from Seed to use for randomized unit tests. "
-             "Special value 'random' means using a seed of None.",
+             "Warn when using `tensor.round` with the default mode. "
+             "Round changed its default from `half_away_from_zero` to "
+             "`half_to_even` to have the same default as NumPy.",
              BoolParam(warn_default('0.9')),
              in_c_key=False)
 
+AddConfigVar(
+    'warn.inc_subtensor1_opt',
+    "Warn if previous versions of Theano (before 0.10) could have "
+    "given incorrect results for inc_subtensor when indexing with "
+    "one array of integers. An incorrect optimization was applied "
+    "when computing set_subtensor(zeros[idx], x)[idx].",
+    BoolParam(warn_default('0.10')),
+    in_c_key=False)
+
 
 AddConfigVar(
     'compute_test_value',

theano/tensor/opt.py

@@ -2960,7 +2960,7 @@ def merge_two_slices(slice1, len1, slice2, len2):
             # sl.stop backwards
             n_val = sl1.stop - 1 - sl2 * sl1.step
             if config.warn.subtensor_merge_bug:
-                _logger.warn((
+                warnings.warn((
                     'Your current code is fine, but Theano versions '
                     'prior to 0.5rc2 might have given an incorrect result. '
                     'To disable this warning, set the Theano flag '
@@ -3473,9 +3473,8 @@ def local_setsubtensor_of_constants(node):
 @register_stabilize
 @gof.local_optimizer([AdvancedSubtensor1])
 def local_adv_sub1_adv_inc_sub1(node):
-    """Optimize the possible AdvSub1(AdvIncSub1(...), ...).
+    """Optimize the possible AdvSub1(AdvSetSub1(...), ...).
 
-    AdvancedSubtensor1(AdvancedIncSubtensor1(0s, y, idx), idx) -> y
     AdvancedSubtensor1(AdvancedSetSubtensor1(x, y, idx), idx) -> y
 
     Notes
@@ -3484,6 +3483,12 @@ def local_adv_sub1_adv_inc_sub1(node):
     index error. If you want to get rid of them, see the
     :ref:`unsafe_optimization` section.
 
+    WARNING:
+    A previous version of this optimization also matched
+    AdvancedSubtensor1(AdvancedIncSubtensor1(0s, y, idx), idx) -> y
+    This is incorrect when there are duplicate indices.
+    The current version warns the user about potential past issues.
+
     """
     if not isinstance(node.op, AdvancedSubtensor1):
         return
@@ -3502,6 +3507,18 @@ def local_adv_sub1_adv_inc_sub1(node):
             # investigate Alloc of 0s but with non constant shape.
             T.extract_constant(x, elemwise=False) != 0):
         return
+
+    if not inp.owner.op.set_instead_of_inc:
+        if config.warn.inc_subtensor1_opt:
+            warnings.warn(
+                'Your current code is fine, but Theano versions '
+                'between 0.7rc1 and 0.10 (or development versions '
+                'between Nov. 2014 and May 2017) '
+                'might have given incorrect results. '
+                'To disable this warning, set the Theano flag '
+                'warn.inc_subtensor1_opt to False.')
+        return
+
     cond = [T.all(T.and_(T.lt(idx, x.shape[0]), T.ge(idx, -x.shape[0])))]
     if not node.fgraph.shape_feature.same_shape(idx, y, 0, 0):
         cond.append(T.eq(idx.shape[0], y.shape[0]))

theano/tensor/tests/test_opt.py

@@ -2658,7 +2658,12 @@ class test_local_adv_sub1_adv_inc_sub1(unittest.TestCase):
 
             dx = np.random.rand(4, 5).astype(dtype1)
             dy = np.random.rand(2, 5).astype(dtype2)
-            didx = np.asarray([1, 3], "int32")
+            # Duplicate the last row of dy
+            dy = np.vstack([dy, dy[-1:]])
+            # Use the same index twice, with the same corresponding value.
+            # That makes set_subtensor well-defined, and tests
+            # duplication for inc_subtensor.
+            didx = np.asarray([1, 3, 3], "int32")
 
             # set_subtensor
             inc = tensor.set_subtensor(x[idx], y)
@@ -2668,11 +2673,8 @@ class test_local_adv_sub1_adv_inc_sub1(unittest.TestCase):
             res = f(dx, dy, didx)
             utt.assert_allclose(dy, res)
             topo = f.maker.fgraph.toposort()
-            if opt:
             assert len(topo) == 1
             assert isinstance(topo[0].op, (compile.DeepCopyOp, T.Elemwise))
-            else:
-                assert len(topo) == 2
 
             # inc_subtensor(data[idx], y)
             inc = tensor.inc_subtensor(x[idx], y)
@@ -2680,7 +2682,9 @@ class test_local_adv_sub1_adv_inc_sub1(unittest.TestCase):
             f = theano.function([x, y, idx], o, self.mode_no_assert)
 
             res = f(dx, dy, didx)
-            utt.assert_allclose((dx[didx] + dy), res)
+            _dx = dx.copy()
+            np.add.at(_dx, didx, dy)
+            utt.assert_allclose(_dx[didx], res)
             topo = f.maker.fgraph.toposort()
             len(topo) == 2
 
@@ -2690,13 +2694,7 @@ class test_local_adv_sub1_adv_inc_sub1(unittest.TestCase):
             f = theano.function([x, y, idx], o, self.mode_no_assert)
 
             res = f(dx, dy, didx)
-            utt.assert_allclose(dy, res)
-            topo = f.maker.fgraph.toposort()
-            if opt:
-                assert len(topo) == 1
-                assert isinstance(topo[0].op, (compile.DeepCopyOp, T.Elemwise))
-            else:
-                assert len(topo) > 2
+            utt.assert_allclose(np.vstack([dy[0], 2 * dy[1], 2 * dy[2]]), res)
 
     def test_assert(self):
             x = tensor.matrix("x")