Incorrect names for multiple identical deterministics

[lpsinger]

If you have multiple deterministics with the same values, then all of their names are incorrectly set to the name of the last deterministic. Here's an example:

>>> import pymc3 as pm
>>> from theano import tensor as tt
>>> with pm.Model() as model:
...     pm.Deterministic('foo', tt.as_tensor_variable(0.0))
...     pm.Deterministic('bar', tt.as_tensor_variable(0.0))
... 
TensorConstant{0.0}
TensorConstant{0.0}
>>> model.foo.name
'bar'
>>> model.bar.name
'bar'

Notice that both deterministics are named bar, whereas the first one should be named foo and the second one named bar.

Versions and main components

• PyMC3 Version: 3.3
• Theano Version: 1.0.1
• Python Version: 3.6.5
• Operating system: macOS 10.13.4
• How did you install PyMC3: MacPorts

[twiecki]

That's a weird one, any idea?

[lpsinger]

It seems like it's the expected behavior from Theano, based on this excerpt from the docstring for tt.as_tensor_variable:

name : str or None
    If a new `Variable` instance is created, it will be named with this
    string.

[lucianopaz]

As @lpsinger pointed out, this is the normal and expected behavior for theano. Theano is basically setting a name to the TensorConstant{0.0}. Given that this node is constant, all of its instances must have the same name or else you could end up with two TensorConstant{0.0} with different names, which would not be interpreted as the same tensor and would break optimizations at the point were the function gets compiled and linked.

Maybe the random variables name should be set in a different attribute too? Something like _name. Then we could either write a get_name function for the Factor class or try to overload the __getattribute__ and __setattr__ methods. I think the latter would be more error prone (and I can't really think of a good way to write __getattribute__ without messing up with theano's compile) while the former is less backward compatible.

class Factor(object):
    ...
    def get_name(self):
        return self._name
    
    def __setattr__(self, attr, value):
        if attr=='name':
            object.__setattr__(self, '_name', value)
            object.__setattr__(self, 'name', value)
        else:
            object.__setattr__(self, attr, value)
    ...

[twiecki]

@lucianopaz Thanks for chiming in. Both options don't seem ideal, the former for the code change required the latter for the unforeseeable consequences. If you're up for it, we could experiment with the second version on a branch and just see if tests still pass.

[lucianopaz]

@twiecki I've tried an MWE that monkey patches a theano expression to handle the __getattribute__ based on the context of whether the call for the name came from within theano or not. My idea was to wrap the theano variable that would be set in the Deterministic function with the monkey patch. I wanted to ask for your feedback before trying out this approach because, quite frankly, what's being done is ugly.

name_test_module.py

import os, inspect, theano
from theano import tensor as tt

__theano_package_path__ = os.path.dirname(theano.__file__)

def wrap_name_attribute(obj, special_name=None):
    klass = obj.__class__
    try:
        has_wrapped_name = klass.__has_wrapped_name__
    except:
        has_wrapped_name = False

    if special_name is None:
        obj.__pymc_name = obj.name
    else:
        obj.__pymc_name = special_name
    
    # Can only wrap a class once
    if not has_wrapped_name:
        try:
            orig_getattribute = klass.__getattribute__
        except:
            orig_getattribute = object.__getattribute__
        try:
            orig_setattr = klass.__setattr__
        except:
            orig_setattr = object.__setattr__
        
        def name_wrapped_getattribute(self, at):
            if at!='name':
                return orig_getattribute(self, at)
            else:
                call_stack = inspect.stack(context=1)
                caller_path = call_stack[1][1]
                if os.path.abspath(caller_path).startswith(__theano_package_path__):
                    return orig_getattribute(self, at)
                else:
                    try:
                        return self.__pymc_name
                    except AttributeError: # This is necessary for ordinary Tensors after the class was monkey patched
                        return orig_getattribute(self, at)
        
        def name_wrapped_setattr(self, at, value):
            if at=='name':
                self.__pymc_name = value
            orig_setattr(self, at, value)
        
        klass.__getattribute__ = name_wrapped_getattribute
        klass.__setattr__ = name_wrapped_setattr
        klass.__unwrapped_getattribute__ = orig_getattribute
        klass.__unwrapped_setattr__ = orig_setattr
        # Flag that the name getting is already wrapped to prevent multiple
        # wrapping, which clobers the inspect.stack and leads to incorrect
        # attribute getting
        klass.__has_wrapped_name__ = True
    
    return obj

def unwrap_name_attribute(obj):
    klass = obj.__class__
    try:
        has_wrapped_name = klass.__has_wrapped_name__
    except:
        has_wrapped_name = False
    if has_wrapped_name:
        klass.__getattribute__ = klass.__unwrapped_getattribute__
        klass.__setattr__ = klass.__unwrapped_setattr__
        delattr(klass, '__unwrapped_getattribute__')
        delattr(klass, '__unwrapped_setattr__')
        delattr(klass, '__has_wrapped_name__')

Then this can be tested with this call:

from name_test_module import *
a = tt.dscalar('a')
b = tt.dscalar('a')
c = tt.dscalar('a')
d = a * b + c

wa = wrap_name_attribute(tt.dscalar('wa'), 'wrapped_name_a')
wb = wrap_name_attribute(tt.dscalar('wa'), 'wrapped_name_b')
wc = wrap_name_attribute(tt.dscalar('wa'), 'wrapped_name_c')
wd = wrap_name_attribute(a * b + c, 'wrapped_name_d')

print('a = {}'.format(a))            # prints a=a
print('a.name = {}'.format(a.name))  # prints a.name=a
print('b = {}'.format(b))            # prints b=a
print('b.name = {}'.format(b.name))  # prints b.name=a
print('c = {}'.format(c))            # prints c=a
print('c.name = {}'.format(c.name))  # prints c.name=a
print('d = {}'.format(d))            # prints d = Elemwise{add,no_inplace}.0
print('d.name = {}'.format(d.name))  # prints d.name = None
tt.printing.debugprint(d)
# prints:
# Elemwise{add,no_inplace} [id A] ''   
# |Elemwise{mul,no_inplace} [id B] ''   
# | |a [id C]
# | |a [id D]
# |a [id E]

print('wa = {}'.format(wa))            # prints wa = a
print('wa.name = {}'.format(wa.name))  # prints wa.name = wrapped_name_a
print('wb = {}'.format(wb))            # prints wb = a
print('wb.name = {}'.format(wb.name))  # prints wb.name = wrapped_name_b
print('wc = {}'.format(wc))            # prints wc = a
print('wc.name = {}'.format(wc.name))  # prints wc.name = wrapped_name_c
print('wd = {}'.format(wd))            # prints wd = Elemwise{add,no_inplace}.0
print('wd.name = {}'.format(wd.name))  # prints wd.name = wrapped_name_d
tt.printing.debugprint(wd)
# prints:
# Elemwise{add,no_inplace} [id A] ''   
# |Elemwise{mul,no_inplace} [id B] ''   
# | |a [id C]
# | |a [id D]
# |a [id E]

You can see that the a.__str__ method tries to get the name from within theano and then prints the name attribute, but when we call a.name from outside of theano, the __pymc_name attribute is returned instead. The problem with this is that it is monkey patching a's class on the fly. I looked a bit around for the order of calls with which Python tries to get an attribute, and from what I understood, the only way to change any theano variable intance's attribute getting was by monkey patching the __class__ on the fly. What do you think about the approach?

[twiecki]

What do you think about the approach?

Well, it seems to work :). It's very unfortunate how complex it is to get this done. I guess this could be done easier if we actually implemented this in Theano? Or, could we alternatively subclass Tensor and overwrite the necessary methods?

[lucianopaz]

Yeah, I think that it would be less error prone to subclass something in theano. Also, I'm not really sure if the monkey patch I did would break model serialization.

The problem I think subclassing will bring is that we should subclass all three base types: theano.tensor.type.TensorType, theano.gof.type.Generic, theano.sparse.type.SparseType. Actually, I'm not sure if this would be enough because I don't really know which class, or classes, work as the abstract base class for everything that gets passed around in theano.

If we go ahead with subclassing some theano class, I think that Deterministic should be changed into some sort of factory function, which inferred which subclass with modified set and get accessors should be used based on the theano expression it got as input. Finally, the inputed theano expression should be reclassed into the modified new subclass. I'll try to look into the possibility of doing something like this with a metaclass in order to not write many subclasses depending on the theano type.

[twiecki]

I'll try to look into the possibility of doing something like this with a metaclass in order to not write many subclasses depending on the theano type.

Thanks @lucianopaz, let me know what you find.