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

Graph Visualization: Added the General Chunk algorithm, multiple stacks to address debug markers, colors for nodes and unit testing. #2550

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

ecapia
Copy link
Contributor

@ecapia ecapia commented Jan 17, 2019

No description provided.

Copy link
Contributor

@bjoeris bjoeris left a comment

Choose a reason for hiding this comment

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

I think some of the functions and data structures are complex enough that they are difficult to understand from the code alone, and some brief comments would probably help with clarity.

func addDebugMarker(builder *labelForVulkanCommands, from, to int) {
level := builder.labelsInsideDebugMarkers[len(builder.labelsInsideDebugMarkers)-1].GetSize() - 1
builder.numberOfDebugMarker++
func (dm *debugMarker) addDebugMarker(from, to int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't really understand how addDebugMarker and checkDebugMarkers are working. A few comments might help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented a different way to handle the debug markers, I think now it is very simple to understand the idea, also I added comments to better understanding.

}
}

type chunk struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this struct and the fields could use brief descriptions of their purpose. The types and names by themselves do not by themselves make it very clear what these variables represent or how they are used. The other structs holding somewhat complex internal data structures like this could also probably use some brief comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a brief descriptions of structs and fields , most of them in regard to the chunk algorithm.

}
}

func (g *graph) joinNodesInChunksByFrame() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that joining nodes by frame, and making the chunks have to be done together like this? Could we have separate joinNodesByFrame() and makeChunks() steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated them in two different functions, join joinNodesByFrame() and makeChunks(), in addition I added the config for the global variables used in the chunk algorithm.

}
}

func makeChunks(nodes []*node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function could use a comment describing its purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment to describe the purpose, also I renamed some variables to better understanding of the code.

@ecapia ecapia force-pushed the graph_visualization_6 branch 2 times, most recently from acf8e73 to 46bd808 Compare January 18, 2019 02:45
Copy link
Contributor

@bjoeris bjoeris left a comment

Choose a reason for hiding this comment

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

lgtm

// for the range [0, 1, 2, 3, ...].
positionToChunksID [][]int

// builded checks if the K-ary tree was builded for this chunk.
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably want built instead of builded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I fixed this, thank you.

Copy link
Contributor

@AWoloszyn AWoloszyn left a comment

Choose a reason for hiding this comment

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

One tiny nit, then its good for me too.

to address debug markers, colors for nodes and unit testing.

General Chunk Algorithm: This algorithm creates super-nodes to avoid
display more than a fix number of nodes in every level. For every
level, the nodes are grouped in super-nodes based on command names,
then if the numbers of nodes exceed the maximum limit, chunks are
created, and if in the new chunks the numbers of nodes exceed the maximum limit,
then new chunks are created and so on.

Results: Applying the General Chunk Algorithm is possible to visualize graphs
containing millions of nodes and edges. The time is reduced from ~20 seconds
to ~2 seconds to expand super-nodes in Tensorboard.

Note1: All "op" attribute for nodes in pbtxt format must be different, since
the find_similar_subgraphs algorithm from Tensorboard computes the similarity
between nodes with same "op".

Note2: Adding colors to the nodes increase the time for expand super-nodes.
@bjoeris bjoeris merged commit a59acde into google:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants