Merge pull request #306 from delallea/sigm_opt_fix

NEWS.txt

@@ -98,6 +98,9 @@ Bug fixes (the result changed):
  * div by zero in verify_grad. This hid a bug in the grad of Images2Neibs. (James)
  * theano.sandbox.neighbors.Images2Neibs grad was returning a wrong value.
    The grad is now disabled and returns an error. (Frederic)
+ * An expression of the form "1 / (exp(x) +- constant)" was systematically matched to "1 / (exp(x) + 1)"
+   and turned into a sigmoid regardless of the value of the constant. A warning will be issued if your
+   code was affected by this bug. (Olivier, reported by Sander Dieleman)
 
 
 Crashes fixed:

doc/NEWS.txt

@@ -98,6 +98,9 @@ Bug fixes (the result changed):
  * div by zero in verify_grad. This hid a bug in the grad of Images2Neibs. (James)
  * theano.sandbox.neighbors.Images2Neibs grad was returning a wrong value.
    The grad is now disabled and returns an error. (Frederic)
+ * An expression of the form "1 / (exp(x) +- constant)" was systematically matched to "1 / (exp(x) + 1)"
+   and turned into a sigmoid regardless of the value of the constant. A warning will be issued if your
+   code was affected by this bug. (Olivier, reported by Sander Dieleman)
 
 
 Crashes fixed:

theano/configdefaults.py

@@ -248,33 +248,38 @@ AddConfigVar('warn.ignore_bug_before',
              EnumStr('None', 'all', '0.3','0.4', '0.4.1', '0.5', allow_override=False),
              in_c_key=False)
 
-default_0_3 = True
-if config.warn.ignore_bug_before == 'None':
-    default_0_3 = True
-elif config.warn.ignore_bug_before == 'all':
-    default_0_3 = False
-elif config.warn.ignore_bug_before >= '0.3':
-    # Disable 0.3 warnings for 0.3 and all subsequent versions
-    default_0_3 = False
+
+def warn_default(version):
+    """
+    Return True iff we should warn about bugs fixed after a given version.
+    """
+    if config.warn.ignore_bug_before == 'None':
+        return True
+    if config.warn.ignore_bug_before == 'all':
+        return False
+    if config.warn.ignore_bug_before >= version:
+        return False
+    return True
+
 
 AddConfigVar('warn.argmax_pushdown_bug',
-             "Warn if in past version of Theano we generated a bug with the optimisation theano.tensor.nnet.nnet.local_argmax_pushdown optimization. Was fixed 27 may 2010",
-             BoolParam(default_0_3),
+             "Warn if in past version of Theano we generated a bug with the theano.tensor.nnet.nnet.local_argmax_pushdown optimization. Was fixed 27 may 2010",
+             BoolParam(warn_default('0.3')),
              in_c_key=False)
 
 AddConfigVar('warn.gpusum_01_011_0111_bug',
              "Warn if we are in a case where old version of Theano had a silent bug with GpuSum pattern 01,011 and 0111 when the first dimensions was bigger then 4096. Was fixed 31 may 2010",
-             BoolParam(default_0_3),
+             BoolParam(warn_default('0.3')),
              in_c_key=False)
 
 AddConfigVar('warn.sum_sum_bug',
              "Warn if we are in a case where Theano version between version 9923a40c7b7a and the 2 august 2010(fixed date), generated an error in that case. This happen when their is 2 consecutive sum in the graph, bad code was generated. Was fixed 2 August 2010",
-             BoolParam(default_0_3),
+             BoolParam(warn_default('0.3')),
              in_c_key=False)
 
 AddConfigVar('warn.sum_div_dimshuffle_bug',
              "Warn if previous versions of Theano (between rev. 3bd9b789f5e8, 2010-06-16, and cfc6322e5ad4, 2010-08-03) would have given incorrect result. This bug was triggered by sum of division of dimshuffled tensors.",
-             BoolParam(default_0_3),
+             BoolParam(warn_default('0.3')),
              in_c_key=False)
 
 AddConfigVar('compute_test_value',

theano/tensor/nnet/sigm.py

@@ -3,18 +3,18 @@
 These functions implement special cases of exp and log to improve numerical stability.
 """
 
+import warnings
 from itertools import imap
 
 import numpy
 
-from theano import gof
-from theano import scalar
-from theano import printing
-from theano.tensor import basic as tensor
-from theano.printing import pprint, debugprint
-from theano.tensor import elemwise
-from theano.tensor import opt
+import theano
+from theano import config, gof, printing, scalar
 from theano.compile import optdb
+from theano.configparser import AddConfigVar, BoolParam
+from theano.printing import pprint, debugprint
+from theano.tensor import basic as tensor
+from theano.tensor import elemwise, opt
 
 
 ############
@@ -166,9 +166,28 @@ def is_1pexp(t):
                         scal_sum = scal_sum + s
                     if numpy.allclose(scal_sum, 1):
                         return False, maybe_exp.owner.inputs[0]
+                # Before 7987b51 there used to be a bug where *any* constant
+                # was considered as if it was equal to 1, and thus this
+                # function would incorrectly identify it as (1 + exp(x)).
+                if config.warn.identify_1pexp_bug:
+                    warnings.warn(
+                        'Although your current code is fine, please note that '
+                        'Theano versions prior to 0.5 (more specifically, '
+                        'prior to commit 7987b51 on 2011-12-18) may have '
+                        'yielded an incorrect result. To remove this warning, '
+                        'either set the `warn.identify_1pexp_bug` config '
+                        'option to False, or `warn.ignore_bug_before` to at '
+                        'least \'0.4.1\'.')
     return None
 
 
+AddConfigVar('warn.identify_1pexp_bug',
+        'Warn if Theano versions prior to 7987b51 (2011-12-18) could have '
+        'yielded a wrong result due to a bug in the is_1pexp function',
+        BoolParam(theano.configdefaults.warn_default('0.4.1')),
+        in_c_key=False)
+
+
 def is_exp(var):
     """
     Match a variable with either of the `exp(x)` or `-exp(x)` patterns.

theano/tensor/nnet/tests/test_sigm.py

@@ -57,6 +57,9 @@ class T_sigmoid_opts(unittest.TestCase):
         x = T.vector()
         data = numpy.random.rand(54).astype(config.floatX)
 
+        backup = config.warn.identify_1pexp_bug
+        config.warn.identify_1pexp_bug = False
+        try:
             # tests exp_over_1_plus_exp
             f = theano.function([x], T.exp(x) / (1 + T.exp(x)), mode=m)
             assert [node.op for node in f.maker.env.toposort()] == [sigmoid]
@@ -138,6 +141,10 @@ class T_sigmoid_opts(unittest.TestCase):
                     T.mul, theano.tensor.inplace.neg_inplace]
             f(data)
 
+        finally:
+            # Restore config option.
+            config.warn.identify_1pexp_bug = backup
+
     def test_1msigmoid(self):
         if not register_local_1msigmoid:
             return
@@ -301,6 +308,9 @@ class T_sigmoid_utils(unittest.TestCase):
                         False, [[False, [[False, x], [False, y]]], [True, z]]]
 
     def test_is_1pexp(self):
+        backup = config.warn.identify_1pexp_bug
+        config.warn.identify_1pexp_bug = False
+        try:
             x = tensor.vector('x')
             exp = tensor.exp
             assert is_1pexp(1 + exp(x)) == (False, x)
@@ -313,3 +323,5 @@ class T_sigmoid_utils(unittest.TestCase):
             assert is_1pexp(exp(x) - 1) is None
             assert is_1pexp(-1 + exp(x)) is None
             assert is_1pexp(1 + 2 * exp(x)) is None
+        finally:
+            config.warn.identify_1pexp_bug = backup