ISISComputingGroup / lewis-ess

Let's write intricate simulators!
GNU General Public License v3.0
21 stars 19 forks source link

Guidelines for Class Attribute vs Instance Attribute usage #235

Open MikeHart85 opened 7 years ago

MikeHart85 commented 7 years ago

Class attributes can be a bit dangerous in Python. They have behaviour that appears very similar to instance attributes most of the time, but work very differently under the hood and can suddenly produce radically different results under some conditions (which may be triggered far away from what is affected).

To illustrate: ```python class Foobar(object): _value = 'Foo' @property def value(self): return self._value # Doesn't really exist until setter is called! @value.setter def value(self, v): self._value = v >>> foo = Foobar() >>> foo.value # Returns Foobar._value (foo._value doesn't exist yet!) 'Foo' >>> foo.value = 'Foo' # Defines foo._value for the first time >>> foo.value # Returns foo._value now 'Foo' # Looks the same, but isn't >>> bar = Foobar() >>> bar.value # Foobar._value 'Foo' >>> Foobar._value = 'Bar' >>> foo.value # foo._value 'Foo' >>> bar.value # Foobar._value still 'Bar' >>> bar.value = bar.value # Side-effects can't be predicted by looking at property code alone (!!!) >>> Foobar._value = 'Baz' >>> bar.value # Now really bar._value 'Bar' ```

Even worse: ```python class Foobar(object): values = [] def add(self, value): self.values.append(value) def get(self): return self.values >>> foo = Foobar() >>> foo.add('hello') >>> foo.get() ['hello'] >>> bar = Foobar() >>> bar.add('world') >>> bar.get() # ?! ['hello', 'world'] >>> foo.get() # !! ['hello', 'world'] >>> foo.values = ['goodbye', 'cruel'] >>> foo.get() ['goodbye', 'cruel'] >>> foo.add('world') >>> foo.get() ['goodbye', 'cruel', 'world'] >>> bar.get() # ?!?! ['hello', 'world'] ``` This is probably not intended behaviour. Again, this is caused by `self.values` not actually existing (and defaulting to Foobar.values). Here with the added twist that, since the value of `values` is mutable, changes to an instance affect all other instances. That is, until I "assign" something to `foo.values` (really, create `foo.values` for the first time and point it at a different list). Now `foo` has its own `values` list, while `bar` still falls back on whatever `Foobar.values` points to.

In C++ terms, Python class attributes appear to behave like static, public data members that automagically become non-static as soon as you assign something to them via this->attribute = ... or instance_variable.attribute = .... But only for that one instance. And not if you modify them any other way apart from assignment (such as calling member functions). And not if you assign to ClassName::attribute instead.

This issue is to do the following:

SimulatedJulabo, in particular, has a lot of class attributes that are used like instance attributes.

chris-d-gregory commented 4 years ago

I appreciate this is old, but it's just tripped me up. When writing a StreamInterface for a device, a Var in the interface command set will not bind to an instance variable of the device, only a class variable. Attempting to do so leads to a an error. I can run the SimulatedJulabo as written without errors, but making the following change:

From this - which runs without error:

class SimulatedJulabo(StateMachineDevice):
    internal_p = 0.1  # The proportional
    internal_i = 3  # The integral
    internal_d = 0  # The derivative
    # etc.

To this:

class SimulatedJulabo(StateMachineDevice):
     def _initialize_data(self):
        self.internal_p = 0.1  # The proportional
        self.internal_i = 3  # The integral
        self.internal_d = 0  # The derivative
        # etc.

results in the following when running:

AttributeError: type object 'SimulatedJulabo' has no attribute 'external_d'

This means that you can't define device attributes (and their initial values) in the _intialize_data method. That's a shame as this method provides a nice way to reset the device. Am I correct, or is there something I'm missing?

MikeHart85 commented 4 years ago

This is an error which I'm surprised we didn't catch before. Need more unit tests!

It's caused in the stream adapter, here and here.

The code inadvertently tries to access the attribute on the type (instead of an instance) of the object. It has to resolve the type, because it's trying to copy the docstrings from @properties. But it shouldn't be failing for things that aren't properties like this.

Thanks for catching this and pointing it out.

I'll make a PR in a sec.

MikeHart85 commented 4 years ago

You should be able to use instance variables the way to wanted if you check out that branch. Let me know how that works for you.

chris-d-gregory commented 4 years ago

I just tested the fix on the SimulatedJulabo and confirm that your mod has resolved my problem. Thanks for your fast response.

MikeHart85 commented 4 years ago

No problem, glad it works for you :+1: