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

Create graph #450

Merged
merged 18 commits into from
Jul 28, 2022
Merged

Create graph #450

merged 18 commits into from
Jul 28, 2022

Conversation

Christian-B
Copy link
Member

@Christian-B Christian-B commented Jul 26, 2022

Builds on #449

An ApplicationGraph is now created during the Writer.setup or mock call. So except for before sim.setup (or unittest_setup())
there is always a graph so no need to call create graph or set_(runtime)_ graph

The Graph Class (or at least what was actually needed) has been merged into ApplicationGraph.
With the following changes

  • The Graph no longer has a label so all graph label params removed
  • You can no longer set or get constraints on graph (Was totally unused)
  • get_edges_ending_at_vertex method removed (no known use)
  • add_vertices and add_edges methods removed as only used in tests
  • Vertex are now only held as the Values in a dict.
    (This has a minor cost that you can not iterate vertices and add at the same time)
  • The allowed Vertex and Edge type to add are now harded coded.

As the Graph is never cloned the clone method has been removed.

As the Graph is never given to the use the ApplicationGraphView is no longer need

Must be done after:
SpiNNakerManchester/SpiNNUtils#184

Must be done at the same time as:
SpiNNakerManchester/SpiNNFrontEndCommon#961
SpiNNakerManchester/sPyNNaker#1202
SpiNNakerManchester/SpiNNakerGraphFrontEnd#217

tested by
http://apollo.cs.man.ac.uk:8080/blue/organizations/jenkins/Integration%20Tests/detail/create_graph/3/pipeline

Documented by
SpiNNakerManchester/SpiNNakerManchester.github.io#42

Copy link
Contributor

@andrewgait andrewgait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, just a few minor edits suggested

pacman/data/pacman_data_view.py Outdated Show resolved Hide resolved
pacman/data/pacman_data_view.py Outdated Show resolved Hide resolved
pacman/model/graphs/application/application_graph.py Outdated Show resolved Hide resolved
@Christian-B Christian-B merged commit 8d87908 into master Jul 28, 2022
@Christian-B Christian-B deleted the create_graph branch July 28, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants