-
Notifications
You must be signed in to change notification settings - Fork 5
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
Global data new #172
Merged
Merged
Global data new #172
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
This was referenced Mar 16, 2022
andrewgait
requested changes
Jul 18, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine in general, just some typos and a question.
andrewgait
approved these changes
Jul 19, 2022
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.
This PR moves a lot of the data held by AbstractSpinnakerBase and its Subclasses out into a Series of Global View Classes.
In Each repository that holds data there is a
_...Model
This holds the actual data and is guaranteed to be a singleton.
_...View
A list of class methods to allow none Simulator access to the data
_...Writer
Allows unittests and the simulator to add global variables.
See the spinn_utilities/data for documentation for all Views and Writers
Ideally the values stored in the GlobalData/View should be accessed directly from the View for every usage.
With no local copies of references and no passing of the Values as parameters.
A clear exception to the not to pass these values as parameters is where the receiving method is also called with different data.
In many case the Algorithms do still keep a local copy/pointer of the values.
This has been done for several reasons.
Once these are in the local copies should be considered for removal.
Even here the could be a later review if it is not better to use the view directly anyway
At least that is my excuse for leaving the pushing down of the View use until later.
With the global data Injections is completely gone.
The use of the GlobalVariables() call to get the simulator has been greatly reduced.
These should now all be action calls and not data calls.
Names of the data have been kept as much as reasonable to the existing names.
While this is a giant PR I am sure there is still much work to be done.
Unless critical I suggest creating issue to address these later.
Extra methods need to simplify the merge with other branches should go in before this is merged.
Especially for the access for graph data.
All There PRs must be done at the same time.
SpiNNakerManchester/SpiNNMachine#170
SpiNNakerManchester/SpiNNMan#270
SpiNNakerManchester/PACMAN#423
SpiNNakerManchester/SpiNNFrontEndCommon#929
SpiNNakerManchester/sPyNNaker#1156
SpiNNakerManchester/SpiNNakerGraphFrontEnd#211
SpiNNakerManchester/MarkovChainMonteCarlo#30
SpiNNakerManchester/SpiNNGym#39
SpiNNakerManchester/SpiNNaker_PDP2#54
SpiNNakerManchester/TestBase#15
SpiNNakerManchester/sPyNNakerNewModelTemplate#82
tested by:
SpiNNakerManchester/IntegrationTests#104
Documented by:
SpiNNakerManchester/SpiNNakerManchester.github.io#41
Future work:
SpiNNakerManchester/SpiNNFrontEndCommon#956
SpiNNakerManchester/SpiNNFrontEndCommon#955
SpiNNakerManchester/SpiNNFrontEndCommon#954