Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NengoObject should error on iteration #1176

Closed
hunse opened this issue Oct 3, 2016 · 8 comments
Closed

NengoObject should error on iteration #1176

hunse opened this issue Oct 3, 2016 · 8 comments
Assignees
Labels

Comments

@hunse
Copy link
Collaborator

hunse commented Oct 3, 2016

A NengoObject should raise an error if you iterate over it. Right now, something like this causes an infinite loop:

u = nengo.Node(1.)
for uu in u:
    print(uu)

This infinite loop will also occur if you try to multiply (or add, etc) u with a Numpy array. This can cause annoying bugs if one accidentally uses just probe instead of sim.data[probe].

I recommend we just raise an error in the __iter__ function for NengoObject.

hunse added a commit that referenced this issue Oct 3, 2016
Accidentally iterating over a NengoObject could be the source of
infinite loops. Fixes #1176.
@jgosmann jgosmann added the bug label Oct 3, 2016
@jgosmann
Copy link
Collaborator

jgosmann commented Oct 3, 2016

I think it might be better to check the dimensions in the __getitem__ method or ObjView constructor and raise an IndexError when appropriate. This would give an early error when trying to do invalid slicing in connections and it should make iteration work as expected.

@hunse
Copy link
Collaborator Author

hunse commented Oct 3, 2016

Be my guest! Personally, I'm not sure if there's a lot of use for iterating over these things, but if you think it's worthwhile and not too difficult to implement and won't cause confusion, then go ahead. If that's something that can be done soon, then it can supercede #1177. Otherwise, I'd suggest going ahead with #1177 until iteration is implemented.

@jgosmann jgosmann self-assigned this Oct 3, 2016
@jgosmann
Copy link
Collaborator

jgosmann commented Oct 4, 2016

Interstingly ObjView is already partially trying to do what I suggest. However, the code does not work as intended as it uses slices which do not raise IndexErrors.

@jgosmann
Copy link
Collaborator

jgosmann commented Oct 4, 2016

Actually it is not entirely clear to me what the desired behaviour in some cases should be. Assume we have a three-dimensional ensemble e = nengo.Ensemble(50, 3) and let us walk through the different cases.

Integer indexing

Valid indices like e[0] should obviously return the appropriate ObjView. Invalid integer indices like e[4] should raise an IndexError to be consistent with how Python indexing works in general and prevent infinite loops (currently no error is raised).

Slice indexing

For slices in range like e[1:3], e[1:], e[:2], e[:] again the appropraite ObjView should be returned. However, it is not clear to me what should happen with out of range slices like e[1:5], e[5:10], e[:10], e[-10:]. Python's standard behaviour is to truncate the out-of-range part of the slice. But maybe we would prefer to raise an error instead? Maybe also because it is not clear in all cases how the truncation behaviour should work. Consider this somewhat contrived example:

e = nengo.Ensemble(50, 2)
c = nengo.Connection(e[1:], post)
e.dimensions = 4

Will c connect from e[1:2] (because initial dimensionality is 2 and the slice gets truncated) or from e[1:4] (because the actual truncation is not done before the build)?

Currently neither of these things might happen. It seems that the code might fail because the slice will not get truncated, but size_in and size_out of ObjView will be calculated based on the truncated slice.

@hunse
Copy link
Collaborator Author

hunse commented Oct 4, 2016

First off, I think Ensemble.dimensions should be read-only. I think that will make our checking easier in general, since an ensemble's size_in and size_out will not change (which will also help for things like encoder validation). Probably n_neurons should be read-only as well, since it defines the sizes of the Neurons object.

That being the case, then I agree that out-of-range integer indices should raise an IndexError. For out-of-range slices, I would truncate them just like Python does.

@jgosmann
Copy link
Collaborator

jgosmann commented Oct 4, 2016

I don't think n_neurons should be read-only, because it would prevent us from implementing #1062 as a helper function. This means we still have to decide on a behaviour with sliced neurons when the neuron number is changed later.

@jgosmann
Copy link
Collaborator

Decisions from the dev meeting:

  • Follow Python: Errors on out of range integer indices, but truncate slices.
  • If dimensions or n_neurons is changed after the slice is created, there will be an error at build time.
  • The discussion whether dimensions and n_neurons should be read-only has been deferred.

@hunse
Copy link
Collaborator Author

hunse commented Oct 31, 2016

Re-thinking about the read-only question, I think it's probably fine to keep them as not read-only. In all our examples, I think we're pretty good about setting parameters when we make objects, either in the constructor or on the attributes right after creation. Maybe we want to add something to the documentation (I'm not sure where, we've talked about a style guide/best practices section before), but I think for the most part all attributes can be set right around creation time, and in that case all validation would work fine.

The one situation where I could see a problem is if someone makes a model and runs it with some number of neurons, and then wants to try it again with a different number of neurons, and in making those changes directly on the model breaks something. But hopefully that's not too common, and the errors will eventually be caught in the builder. And there would be lots of times that wouldn't break anything and would be a useful feature, so might as well allow it.

We can always re-consider pre-build validation, too. Maybe there's an easy way to do that by re-calling the "validate" functions on all the parameters, or maybe just the ones we're worried about.

jgosmann added a commit that referenced this issue Oct 31, 2016
Integer indices out of bounds should raises an IndexError, whereas
slices should get truncated silently.

This has the added benefit of allowing iteration over Nengo objects.

Fixes #1176.
tbekolay pushed a commit that referenced this issue Feb 7, 2017
Integer indices out of bounds should raises an IndexError, whereas
slices should get truncated silently.

This has the added benefit of allowing iteration over Nengo objects.

Fixes #1176.
adityagilra pushed a commit to adityagilra/nengo that referenced this issue Jun 21, 2017
Integer indices out of bounds should raises an IndexError, whereas
slices should get truncated silently.

This has the added benefit of allowing iteration over Nengo objects.

Fixes nengo#1176.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants