Devices2

[mstimberg]

How about merging this into master, to close #114 and #122?

[coveralls]

Coverage remained the same when pulling 399479e on devices2 into ac71ad0 on master.

[mstimberg]

Wasn't patient enough for the travis run to finish, so I added two more changes which should close #84: Group now derives from BrianObject (this would lead to the weird situation that classes deriving from Group would often call BrianObject.__init__ in the beginning and Group.__init__ in the end, I therefore renamed Group.__init__ to Group._enable_group_attributes, so classes now only call Group.__init__ once, corresponding to the former BrianObject.__init__).
I also removed the "set dt only once" restriction that is no longer necessary in Brian 2.

[coveralls]

Coverage remained the same when pulling 81356e9 on devices2 into ac71ad0 on master.

[thesamovar]

Great.

I think we should roll back the dt change though. It's not written in #84 (I guess we discussed by email), but there's an unresolved issue with SpikeQueue that would need to be solved first I think.

[thesamovar]

Also I'm not sure that having Group._enable_group_attributes is simpler than Group.__init__ having to be called at the end? No strong feelings about that though.

[mstimberg]

Also I'm not sure that having Group._enable_group_attributes is simpler than Group.init having to be called at the end? No strong feelings about that though.

Sorry, I did not express this very clearly. It's not about Group.__init__ being complicated. Before, a class like NeuronGroup inherited from Group and BrianObject and called BrianObject.__init__ in the beginning and Group.__init__ in the end. Now, NeuronGroup only inherits from Group, therefore calling BrianObject.__init__ directly and bypass Group.__init__ looked strange to me.
Old:

class NeuronGroup(BrianObject, Group):
    def __init__(self, ...):
         BrianObject.__init__(self, name=..., when=...)
         ....
         Group.__init__(self)

New:

class NeuronGroup(Group):
    def __init__(self, ...):
         Group.__init__(self, name=..., when=...)
         ....
         self._enable_group_attributes()

[thesamovar]

Yep, understood. Just wondering if maybe we should keep Group as not deriving from BrianObject given that it doesn't save any typing to make Group derive from BrianObject. It actually makes it slightly longer. Also, whereas it's clear that if you derive from two classes you need to call __init__ on both classes, it's not clear that you need to call _enable_group_attributes.

[coveralls]

Coverage remained the same when pulling 9e72b25 on devices2 into ac71ad0 on master.

[mstimberg]

About the changing dt: It turned out to be easier than expected to make it work. I pushed a change that reads out all the spikes from the queue and reinserts them using the new dt. It seems to be working fine (I added a test). Of course it's not retrospectively using a finer temporal resolution if dt got smaller. But I don't see any way to make this work with any reasonable effort and it's a real corner case.

About BrianObject and Group: I think it's not entirely intuitive to call two __init__ functions, even if inheriting from two classes -- the "right thing to do[TM]" would be to call __init__ with super and that would automatically call the two superclasses. This is not working in this case since we have to call one at the beginning, the other at the end. Given that the user (or rather developer) has to be explicitly aware of switching on the group access at the end of __init__ function, I think it's clearer to do this with a clearly named function call instead of with Group.__init__.

That all said: I'm not sure whether we really need BrianObject and CodeObject (and Updater) -- couldn't we do some merging so that it is clear what the "object that is executed during a run" is?

[thesamovar]

Cool! I thought this would be a lot more work to get the changing dt thing going. OK, let's keep this then.

For BrianObject and Group - OK I'm happy with your solution. Here's just one quick alternative, though I suspect you won't like it because it uses metaclasses. ;)

Basically, you use a metaclass to define group attribute access rather than a class, and this metaclass automatically modifies the __init__ method to call the group attribute access activation after the __init__ completes. Here's the code:

def group_metaclass(name, parents, attributes):
    if Group not in parents:
        parents = parents+(Group,)
    obj = type(name, parents, attributes)
    obj._orig_init = obj.__init__
    def newinit(self, *args, **kwds):
        self._orig_init(*args, **kwds)
        self._after_init()
    obj.__init__ = newinit
    return obj

class Group(object):
    def _after_init(self):
        print 'AfterInit'

class Base(object):
    pass

class X(Base):
    __metaclass__ = group_metaclass
    def __init__(self):
        print 'XInit'

x = X()

This could be used in standard cases, and in cases where you needed more control you could not use the metaclass but rather derive from Group and call the _enable_group_attributes by hand when it was necessary.

On the last point, yes we'll definitely want to simplify the whole BrianObject/CodeObject/Updater nonsense, but let's not do that now.

[coveralls]

Coverage remained the same when pulling 8a81ce8 on devices2 into ac71ad0 on master.

[mstimberg]

Cool! I thought this would be a lot more work to get the changing dt thing going. OK, let's keep this then.

I was surprised myself -- but apparently a lot of the code already took a possibly changing dt into account, it was really only about the spikes in the queue. The code is not performance optimised at all (before_run doesn't even check whether dt actually changed) -- but I don't think this will take any noticeable amount of time, anyway.

About Group and BrianObject: I'd actually prefer something more automatic like your metaclass approach. Maybe implemented as a class decorator instead? I'm only worried about negative side effects like: what does the stack trace for an error in __init__ look like, will this be confusing to users when it complains about a wrong argument, for example?

And I agree that we shouldn't do the cleaning up now. If you prefer, we can also revert the BrianObject/Group merge for now, until we come up with a really nice solution.

[coveralls]

Coverage remained the same when pulling 00cc1d4 on devices2 into ac71ad0 on master.

[thesamovar]

Good idea, class decorator is a much nicer idea than metaclass. Happy to keep the BrianObject/Group merge if we have an automatic approach in mind as a future improvement. I'm happy to merge this if you're happy?

[thesamovar]

Actually I'm going to commit some changes to this branch that we won't want to merge immediately so I'm going to merge this now.

[mstimberg]

Happy to keep the BrianObject/Group merge if we have an automatic approach in mind as a future improvement. I'm happy to merge this if you're happy?

It's too late now, but I am happy :) I filed an issue for the metaclass/class decorator idea at #130.