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

Support multiple use of computation graph vertex as input to other vertices #7420

Merged
merged 2 commits into from Apr 2, 2019

Conversation

Projects
None yet
2 participants
@treo
Copy link
Member

commented Apr 1, 2019

While building a test case for the attention vertex, I've stumbled across the use case were I needed to use the same vertex output three times as the input.

In that specific case it was that I was using the output of an lstm for queries, keys and values.

The reason why it was failing was that indexOf will always only find the first appearance of an entry within a list.

@treo treo requested a review from AlexDBlack Apr 1, 2019

@AlexDBlack
Copy link
Member

left a comment

Let's gradient check this situation also. The change looks reasonable at first glance, but I'm not confident it'll be fine during training...

@@ -644,15 +646,17 @@ public void init(INDArray parameters, boolean cloneParametersArray) {
continue; //Output vertex
VertexIndices[] outputIndices = new VertexIndices[thisVertexOutputsTo.size()];
int j = 0;
for (String s : thisVertexOutputsTo) {
for (String s : new HashSet<>(thisVertexOutputsTo)) {

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Apr 2, 2019

Member

I don't think we need this new hashset construction here... we're not modifying thisVertexOutputsTo in the loop, so it should be fine as-is?

This comment has been minimized.

Copy link
@treo

treo Apr 2, 2019

Author Member

thisVertexOutputsTo contains duplicates that we need in order to calculate the size of outputIndices correctly. But once we start iterating over it we are only interested in the unique values. Creating a HashSet was just the smallest change that I could think of.

@treo

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Sure, I'll add gradient checks. It is how I ran into the issue in the first place (i.e. while creating gradient checks for the AttentionVertex)

@treo treo merged commit 4f4a075 into master Apr 2, 2019

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@treo treo deleted the treo/cg_reuse_inputs branch Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.