-
Notifications
You must be signed in to change notification settings - Fork 7
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
Extdev fpgas #463
Extdev fpgas #463
Conversation
Conflicts: pacman/operations/algorithms_metadata.xml
This reverts commit 6a61243.
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.
Lots of small comments. None are blockers but all should be address at some point (ok for version 2)
pacman/model/graphs/application/abstract/abstract_2d_device_vertex.py
Outdated
Show resolved
Hide resolved
pacman/model/graphs/application/abstract/abstract_2d_device_vertex.py
Outdated
Show resolved
Hide resolved
pacman/model/graphs/application/abstract/abstract_2d_device_vertex.py
Outdated
Show resolved
Hide resolved
pacman/utilities/algorithm_utilities/routing_algorithm_utilities.py
Outdated
Show resolved
Hide resolved
pacman/operations/routing_table_generators/external_device_null_routing_table_generator.py
Outdated
Show resolved
Hide resolved
pacman/operations/routing_table_generators/external_device_null_routing_table_generator.py
Outdated
Show resolved
Hide resolved
pacman/utilities/algorithm_utilities/partition_algorithm_utilities.py
Outdated
Show resolved
Hide resolved
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.
I'm happy with this; just made one more comment here that might be worth thinking about.
|
||
SETTING_SPLITTER_ERROR_MSG = ( | ||
"The splitter object on {} has already been set, it cannot be " | ||
"reset. Please fix and try again. ") | ||
|
||
def __init__(self, label=None, constraints=None, | ||
max_atoms_per_core=sys.maxsize, splitter=None): | ||
max_atoms_per_core=None, splitter=None): |
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.
I wonder whether calling this variable max_atoms_per_dimension_per_core as well might be a good idea. (I'm not sure whether PyNN has named this variable already and that might mean that might not be a good idea...). I suppose because in the majority of cases it will still be a single int that it's probably not worth changing but I thought I'd put the suggestion out there.
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.
I did wonder the same thing but didn't for two reasons:
- It may be hard to find places where this is used as a keyword argument given the number of application vertices around, and how some might be called via super(...).
- Arguably, max_atoms_per_dimension_per_core returned elsewhere is slightly different, in that it is always a tuple, where this can be a tuple, int or None.
The main change here is to support external devices connected via the FPGAs and the addition of 2D support to vertices. The main highlights:
Depends on SpiNNakerManchester/SpiNNMachine#181