Make Nengo-specific Exceptions

[tbekolay]

I was adding a helpful exception when trying to probe decoders or transform, which will be removed in #729, but in order to get that exception working nicely, it kind of ballooned into reworking the exceptions currently raised for validation errors. This should be less magical / more clear, but perhaps more verbose (that might be fixable though).

But yeah, this is also a work in progress, I'm just making this PR now because I'm going to refer to this in #729.

[tbekolay]

Note to self: Also add an ObsoleteError for getting the 'decoders' in the BuiltConnection.

[jgosmann]

Note to self: Also add an ObsoleteError for getting the 'decoders' in the BuiltConnection.

Might DeprecatedError be a ebetter name? At least to me it seems that "deprecated" is more widely used than "obsolete".

[hunse]

We discussed this somewhere, and @arvoelke pointed out that deprecated things are still usable, they'll just be removed in a future version (or otherwise made unusable in the future). He suggested "obsolete" to refer to these things that are unusable now.

[hunse]

The thesaurus suggests AntediluvianError, HorseAndBuggyError, MothEatenError, or PasséError.

[hunse]

The only other thing I can think of is RemovedError and RemovedParam (though it sounds a bit like we got rid of an error).

[tbekolay]

Yeah, Removed is the only other real synonym for the reason @hunse pointed out, but Removed has too many other possible meanings, I think. Also, we already have the ObsoleteParam so if we want to rename the error, we have to rename the param too.

[jgosmann]

Ah, I forgot about the distinction @arvoelke pointed out. With that ObosoleteError makes sense. :)

[Seanny123]

I'd like an error and a warning for things that aren't biologically plausible. My motivation is that in SPA, there's a lot of operations that we don't allow that we cover with NotImplemented when really it should be NeverWillBeImplemented because it really isn't biologically plausible.

For example, in spa.Actions doesn't parse every possible string you can throw at it because a lot of options would mean creating new networks, which isn't a direction we want to push the user in, as discussed in #759 .

[tcstewar]

I'd like an error and a warning for things that aren't biologically plausible. My motivation is that in SPA, there's a lot of operations that we don't allow that we cover with NotImplemented when really it should be NeverWillBeImplemented because it really isn't biologically plausible.

For example, in spa.Actions doesn't parse every possible string you can throw at it because a lot of options would mean creating new networks, which isn't a direction we want to push the user in, as discussed in #759 .

I think that'd be a SPA-specific exception rather than a Nengo-specific exception, since it's much more about SPA design philosophy (as much as that can be said to exist....)

[tbekolay]

Note to self: add SimulationClosedError as per #859.

[tbekolay]

This PR's ready for review. The main thing I don't like is the ValidationError in a4cdcc0 ... it's a better user experience, as it removes a bit of magic and gives informative messages in more situations. But for devs, it's kind of verbose / annoying. So, closer looks at that commit, and alternative suggestions would be appreciated!

[hunse]

What don't you like about ValidationError? Just that it takes a lot of arguments, that the dev has to figure out? I think it's an improvement. It's really only core Nengo devs that will have to deal with it I think. People writing backends can avoid it, as can people writing Nengo extensions (e.g. new neruon types) since they can just use our parameters and throw ValueErrors if they need extra exceptions.

[tbekolay]

The two things that irk me a bit about ValidationError are:

• Needing to access __class__ a lot, which seems a bit clunky to me. But this could maybe be helped by accepting an instance and automatically doing __class__ if it's not a class?
• The redundancy in specifying the same name twice for each param; e.g., tau = NumberParam('tau', ...)

But you're right, it's only Nengo devs that have to care about it, so I would say my second concern is no biggie. But I'll try out accepting an instance and introspecting a bit to do the __class__ part automatically.

Also, what do you think about where ValidationError is raised? Often I don't really care to see the validate method, I mostly care about a few levels up the stack where I passed the wrong argument to the Nengo object. Should we reraise ValidationErrors a few levels up the stack?

[hunse]

I'll try out accepting an instance and introspecting a bit to do the __class__ part automatically.

Sounds good, I think that would be an improvement. I was trying to think of a case where that magic would be troublesome, but nothing is coming to me right now.

Often I don't really care to see the validate method, I mostly care about a few levels up the stack where I passed the wrong argument to the Nengo object. Should we reraise ValidationErrors a few levels up the stack?

I agree, you usually want to see it where the parameter is set. As long as the error message is more than clear enough for the user to fix it; if the message is unclear, then it can be nice to see the actual logic that's leading to the error.

How hard would it to be to reraise these things? If it's not too hard, I think it would be worth trying.

Overall, this looks good to me. I was thinking I should test things out and look at some of the error messages, and then I started wondering whether it's worth having a notebook that raises many/most of the errors we have in Nengo so it's easy to look at all the messages. We could organize it into sections based on file.

[tbekolay]

a notebook that raises many/most of the errors we have in Nengo so it's easy to look at all the messages

Yeah, sounds good. I think something like that's necessary to test the reraise logic anyhow so I'll give it a shot! Thinking it will be kind of like the strings notebook.

[tbekolay]

OK, so I've been playing around with this a bit. It is possible to reraise specifically for validation errors for NengoObjects only, but it's not perfect. I would appreciate votes on which of the two possibilities are better from the user perspective!

Option 1: No reraising

In a bare Python interpreter

>>> with nengo.Network():
...     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "nengo/base.py", line 30, in __call__
    inst.__init__(*args, **kwargs)
  File "nengo/ensemble.py", line 89, in __init__
    self.n_neurons = n_neurons
  File "nengo/base.py", line 69, in __setattr__
    super(NengoObject, self).__setattr__(name, val)
  File "nengo/params.py", line 186, in __set__
    super(NumberParam, self).__set__(instance, value)
  File "nengo/params.py", line 80, in __set__
    self.validate(instance, value)
  File "nengo/params.py", line 217, in validate
    super(IntParam, self).validate(instance, num)
  File "nengo/params.py", line 200, in validate
    num))
nengo.exceptions.ValidationError: Ensemble.n_neurons: Value must be greater than or equal to 1 (got 0)

In IPython

In [2]: with nengo.Network():
   ...:     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
   ...:     
---------------------------------------------------------------------------
ValidationError                           Traceback (most recent call last)
<ipython-input-2-46a60ac68203> in <module>()
      1 with nengo.Network():
----> 2     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
      3 

/home/tbekolay/Code/nengo/nengo/base.pyc in __call__(cls, *args, **kwargs)
     28         add_to_container = kwargs.pop('add_to_container', True)
     29         # Do the __init__ before adding in case __init__ errors out
---> 30         inst.__init__(*args, **kwargs)
     31         if add_to_container:
     32             Network.add(inst)

/home/tbekolay/Code/nengo/nengo/ensemble.pyc in __init__(self, n_neurons, dimensions, radius, encoders, intercepts, max_rates, eval_points, n_eval_points, neuron_type, gain, bias, noise, seed, label)
     87                  bias=Default, noise=Default, seed=Default, label=Default):
     88 
---> 89         self.n_neurons = n_neurons
     90         self.dimensions = dimensions
     91         self.radius = radius

/home/tbekolay/Code/nengo/nengo/base.pyc in __setattr__(self, name, val)
     67             val = Config.default(type(self), name)
     68         # try:
---> 69         super(NengoObject, self).__setattr__(name, val)
     70         # except ValidationError:
     71         #     exc_info = sys.exc_info()

/home/tbekolay/Code/nengo/nengo/params.pyc in __set__(self, instance, value)
    184         if is_array(value) and value.shape == ():
    185             value = value.item()  # convert scalar array to Python object
--> 186         super(NumberParam, self).__set__(instance, value)
    187 
    188     def validate(self, instance, num):

/home/tbekolay/Code/nengo/nengo/params.pyc in __set__(self, instance, value)
     78 
     79     def __set__(self, instance, value):
---> 80         self.validate(instance, value)
     81         self.data[instance] = value
     82 

/home/tbekolay/Code/nengo/nengo/params.pyc in validate(self, instance, num)
    215             raise ValidationError(instance.__class__, self.name,
    216                                   "Must be an integer; got '%s'" % num)
--> 217         super(IntParam, self).validate(instance, num)
    218 
    219 

/home/tbekolay/Code/nengo/nengo/params.pyc in validate(self, instance, num)
    198                         "" if self.low_open else "or equal to ",
    199                         self.low,
--> 200                         num))
    201             high_comp = 0 if self.high_open else 1
    202             if self.high is not None and compare(num, self.high) >= high_comp:

ValidationError: Ensemble.n_neurons: Value must be greater than or equal to 1 (got 0)

Option 2: Reraising

In a bare Python interpreter

>>> with nengo.Network():
...     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "nengo/base.py", line 30, in __call__
    inst.__init__(*args, **kwargs)
  File "nengo/ensemble.py", line 89, in __init__
    self.n_neurons = n_neurons
  File "nengo/base.py", line 73, in __setattr__
    reraise(exc_info[0], exc_info[1], None)
  File "<string>", line 2, in reraise
nengo.exceptions.ValidationError: Ensemble.n_neurons: Value must be greater than or equal to 1 (got 0)

In IPython

In [2]: with nengo.Network():
   ...:     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
   ...:     
---------------------------------------------------------------------------
ValidationError                           Traceback (most recent call last)
<ipython-input-2-46a60ac68203> in <module>()
      1 with nengo.Network():
----> 2     ens = nengo.Ensemble(n_neurons=0, dimensions=1)
      3 

/home/tbekolay/Code/nengo/nengo/base.pyc in __call__(cls, *args, **kwargs)
     28         add_to_container = kwargs.pop('add_to_container', True)
     29         # Do the __init__ before adding in case __init__ errors out
---> 30         inst.__init__(*args, **kwargs)
     31         if add_to_container:
     32             Network.add(inst)

/home/tbekolay/Code/nengo/nengo/ensemble.pyc in __init__(self, n_neurons, dimensions, radius, encoders, intercepts, max_rates, eval_points, n_eval_points, neuron_type, gain, bias, noise, seed, label)
     87                  bias=Default, noise=Default, seed=Default, label=Default):
     88 
---> 89         self.n_neurons = n_neurons
     90         self.dimensions = dimensions
     91         self.radius = radius

/home/tbekolay/Code/nengo/nengo/base.pyc in __setattr__(self, name, val)
     71             exc_info = sys.exc_info()
     72             logger.debug(traceback.format_tb(exc_info[2]))
---> 73             reraise(exc_info[0], exc_info[1], None)
     74 
     75     def __getstate__(self):

/home/tbekolay/Code/nengo/nengo/utils/compat.pyc in reraise(tp, value, tb)

ValidationError: Ensemble.n_neurons: Value must be greater than or equal to 1 (got 0)

---

As far as I can tell, there's no way to get rid of reporting that the exception is reraised (ideally the self.n_neurons = n_neurons line would be where the traceback starts) but since it's much shorter, I'm in favor of option 2 (reraise). Note that I log the full traceback with logger.debug so that if someone's editing the Parameter code it's easier to debug.

[hunse]

You're right, neither one seems perfect. But I agree that option 2 seems better because of the shorter traces.

Would it be possible to have this as a config option? This seems like something that people might want to customize, defaulting to option 2.

[tbekolay]

OK just pushed some new stuff to address the typo and to remove ZeroActivityError. I think for the time being I'm going to leave NetworkContextError and SpaModuleError, but we can always come back to it in another issue / PR.

[drasmuss]

Looks good to me. I'll wait for a +1 from @hunse, then I can merge. Do you have plans in mind for the history, or want me to just squash all the fixups?

[tbekolay]

I think I would favor just squashing the fixups, as each commit has a pretty clear commit message that should be easy to ignore when looking through the commit log for non-exception stuff. But if other people think it's too much we could squash the exceptions from BuildError to SpaModuleError into one big commit (so 5 commits total, something like 5373328 bc633eb ba93b8d b3fcaab 51c149f). Reviewer's choice ;)

[hunse]

I'm fine keeping things separate. My only caveat is I'd like the changelog entry part in e155633 to appear at the end, because it's describing the changes of all the commits. Everything LGTM, btw.

[tbekolay]

I'll split that out now and move it to the end.

[tbekolay]

Done; marked it as a FIXUP on d470d34 so should probably add a note about that to its commit message.