Copy fixes #1271
Merged
Copy fixes #1271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The keys of the WeakKeyIDDictionaries do not get updated on a normal copy operation. This is rarely a problem because Parameters are usually class variables that will not get copied, but there is the possibility to define instance parameters (slightly obscure usage, but is for example used in the current SPA system). In this case the Parameter instance needs to be copied. Adding the __getstate__ and __setstate__ handler allows to convert the WeakKeyIDDictionaries to normal dictionaries with strong references and convert them back. This will cause the keys to be updated (partially due to Python's copy magic). It also seems to me that obtaining strong references during the copy process is a good idea because than none of the objects can disappear during copying (or serialization). Addresses #1266.
The involved __getattr__/__setattr__ logic would otherwise screw up. Addresses #1266.
When reconstructing objects during copy, the check for whether a module has been added twice to a model needs to be avoided. Addresses #1266.
jgosmann
added a commit
to nengo/nengo-spa
that referenced
this pull request
May 9, 2017
3 tasks
jgosmann
added a commit
to nengo/nengo-spa
that referenced
this pull request
May 10, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and context:
It was brought to my attention that copying of SPA models did not work (#1266). While investigating, it became apparent that only part of the problem is SPA specific. The other part is related to copying instance parameters. This are a somewhat obscure feature and rarely used, but unfortunately the current SPA system depends on them (e7b0edc).
This PR fixes all those problems.
Interactions with other PRs:
It might be necessary to fix this in the new SPA system too. Thus, I would like to keep the associated issue #1266 open.
How has this been tested?
Added two tests. One for copying instance parameters and one for copying a SPA model.
How long should this take to review?
Not many lines have changed which can make this a normal length review, but the config and param system is quite complex and intricated. So depending on prior familarity with it and how much one desires to understand how all the pieces work together, this can get a length review.
Where should a reviewer start?
This is a bit difficult. I split this up in multiple commits concerned with specific things, but all of these commits (except the last SPA commit) are needed just to fix the copying of instance params.
Types of changes:
Checklist:
Still to do: