nengo / nengo-ocl

OpenCL-based simulator for Nengo neural models
Other
23 stars 14 forks source link

Nengo OCL fails with Nengo 2.3.1 #132

Closed tbekolay closed 7 years ago

tbekolay commented 7 years ago

Prepping the 2.3.1 release and getting the following failure when building with Nengo OCL:

../nengo_ocl/nengo_ocl/simulator.py:228: in __init__
    operators = list(map(MultiDotInc.convert_to, operators))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'nengo_ocl.operators.MultiDotInc'>
op = <Copy"<Connection from <Node (unlabeled) at 0x7f7c040e9710> to <Ensemble "0">>.gain" at 0x7f7bfc7d8110>

    @classmethod
    def convert_to(cls, op):
        if isinstance(op, Copy):
            rval = cls(op.dst, op.src, beta=1, gamma=0, tag=op.tag)
        elif isinstance(op, DotInc):
            rval = cls(op.Y, op.Y, beta=1, gamma=0, tag=op.tag)
            rval.add_AX(op.A, op.X)
        else:
            return op

        assert set(op.reads).issuperset(rval.reads), (rval.reads, op.reads)
>       assert rval.incs == op.incs
E       AssertionError

../nengo_ocl/nengo_ocl/operators.py:47: AssertionError

This happens for all models, so the specific test isn't relevant. Since this deals with the Copy operator, it seems quite likely that the issue is related to https://github.com/nengo/nengo/pull/1083 though https://github.com/nengo/nengo/pull/1245 also touches the op graph so that might play a role.

Let me know if this is an easy fix that I can do @hunse, or if you want me to hold off on the Nengo release.

hunse commented 7 years ago

This was easier than I expected to fix. Take a look at #133 and if you're happy, go ahead and merge it.

I have one request, though: Can you make sure that the tests don't take significantly longer with the change, and that the benchmark in examples/benchmark_circconv.py also doesn't take longer? I just want to make sure we're not losing anything by not putting the Copy into MultiDotInc.

tbekolay commented 7 years ago

Take a look at #133 and if you're happy, go ahead and merge it.

Your changes look good to me; I added some commits to keep compatibility with older Nengo versions and a few other things; take a look and merge away if you're OK with them.

Can you make sure that the tests don't take significantly longer with the change, and that the benchmark in examples/benchmark_circconv.py also doesn't take longer?

Done! tl;dr: On my machine there's very little difference in speed. Details follow:

Running the tests

benchmark_cconv.py

plan_copy might be a tiny bit slower, but in general pretty close:

plan_copy_bench