Allow FrozenObjects to control param init order #1493
Conversation
|
I think this warrants to revive the discussion whether to have the copy code in a mixin class. |
|
I don't have a strong opinion one way or the other, but it would be nice to get this resolved quickly as this is breaking NengoDL |
| if isinstance(v, Parameter) and not isinstance(v, ObsoleteParam)} | ||
| self._paramdict = collections.OrderedDict( | ||
| (k, v) for k, v in inspect.getmembers(type(self)) | ||
| if isinstance(v, Parameter) and not isinstance(v, ObsoleteParam)) |
There was a problem hiding this comment.
Just taking a guess what you're going for here, but keep in mind that inspect.getmembers returns the members in alphabetical order, not the order they're defined in the code.
There was a problem hiding this comment.
Yes, I think that's fine. I just wanted to make them have a consistent order, so that we can rely on some order. Does that make sense?
There was a problem hiding this comment.
I've added a few little fixups. One makes sure the parameters always have a consistent order, just to make things consistent. It also saves a bit of time in __repr__ because we don't have to sort them then.
The other changes are just minor ones for speed: not turning an iterator into a list (since we always just iterate over it), and not having to check if the param is in state each time through the loop in __setstate__.
This looks ready to go to me. I don't think we need to hold this up for a discussion about making copying a mixin; that's a discussion that can happen on its own.
|
Fixups look good to me. |
nengo/params.py
Outdated
| for p in self._params: | ||
| if not p.readonly: | ||
| msg = "All parameters of a FrozenObject must be readonly" | ||
| raise ReadonlyError(attr=p, obj=self, msg=msg) | ||
|
|
||
| @property | ||
| def _params(self): | ||
| return list(itervalues(self._paramdict)) | ||
| return itervalues(self._paramdict) |
There was a problem hiding this comment.
So the reason this was a list and not an iterator is because it's a property. When you do thing.attribute you do not expect to get back an iterator, conventionally iterators are only returned from methods (hence why we have dict.values() etc rather than dict.values). This is important because:
param = self._params
for p in param:
# this works fine
for p in param:
# this now fails because the iterator is exhaustedI definitely wouldn't expect this behavior, as an attribute should just give me data. I would be more likely to expect something like this with params = self._iterparams().
tbekolay
force-pushed
the
frozen-order
branch
from
73c879f
to
2904516
Compare
tbekolay
force-pushed
the
frozen-order
branch
from
2904516
to
9713a29
Compare
Motivation and context:
With our Parameter validation system, sometimes parameters need to be initialized in a particular order. When copying an object the order is random by default, so we need to impose an order on this. We had this in place for
NengoObjects, but not forFrozenObjects. Previously this was fine because we didn't have anyFrozenObjectswhere order mattered, but for the newConvolutionobject it does matter.I also added a seed for the Convolution test. I had a random test failure at some point that I wasn't able to reproduce again, so I'm not even 100% sure if it was real. But adding a seed here doesn't hurt.
How has this been tested?
Added a new test that fails without this change and passes with it.
How long should this take to review?
Types of changes:
Checklist: