Raise BuildError if conn function returns None #1319
Conversation
| targets[i] = conn.function(ep) | ||
| out = conn.function(ep) | ||
| if out is None: | ||
| raise BuildError("Building %s: Connection function returned " |
There was a problem hiding this comment.
The error message is good! I am just wondering if those memory locations (and other meta-data about Ensembles) is really necessary, e.g.,:
BuildError: Building <Connection from <Ensemble (unlabeled) at 0x7fe738f619d0> to <Ensemble (unlabeled) at 0x7fe738f61b90> computing 'f'>: Connection function returned None. Cannot solve for decoders.
I find they obfuscate the message, but if this is standard in error reporting for Nengo, then it's probably better to leave it as it is.
There was a problem hiding this comment.
The memory locations go away if the pre and post of the connection have labels. I agree that they're not necessary for most purposes, but they are useful for debugging... might be worth making them something that show up in debug mode only?
There was a problem hiding this comment.
The memory locations go away if the pre and post of the connection have labels.
Cool, I didn't know connections can have labels! :)
might be worth making them something that show up in debug mode only?
Yeah, (if it's not a hassle to implement) I'd say so. The error msg that appears when connections have labels is way clearer to me. I don't think we used label keyword once during the summer school tutorials so I am guessing someone new to Nengo will most likely end up with that long error, which is more useful to devs than regular users.
There was a problem hiding this comment.
Personally I prefer to have the memory addresses in the error message for unlabeled objects because it is the only thing maybe allows me to identify the object. Adding a “debug mode” might make the error message to appear simpler, but it will require users to know about the debug mode and it is likely to introduce more complexity to the code base.
There was a problem hiding this comment.
I'll make a separate PR for it so we can discuss it in more depth there.
Doing this as model definition time is possible, but requires running the function, which may not be desirable.
tbekolay
force-pushed
the
error-for-none-function
branch
from
4b33f1d
to
31b9c20
Compare
Motivation and context:
In #1318 @ikajic reported that we don't raise a nice error if someone forgets to return a value from a connection function (which is a common thing to forget, especially when moving from a lambda to a named function). This raises a nice error when that happens.
How has this been tested?
Added a test that the error gets raised.
How long should this take to review?
Types of changes:
Checklist:
Discussion points
This error gets raised at build time, not a model creation time. This is because running a connection function might not be something the user expects when defining the model. It already gets run at build time, so this keeps the user's expectations the same. We could do this at model creation time though, if the consensus prefers that.