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

[GraphBuilder] Use last added layer/vertex if no inputs specified #7403

Merged

Conversation

Projects
None yet
2 participants
@rnett
Copy link
Contributor

commented Mar 30, 2019

What changes were proposed in this pull request?

When building a computation graph with ComputationGraphConfiguration.GraphBuilder, when you add a layer and don't specify an input, use the last added layer/vertex (if you have previously added at least one).

How was this patch tested?

Removed test that expected exception that now will not be thrown.
Changed one other test to use this method of adding inputs.

rnett added some commits Mar 29, 2019

@AlexDBlack

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

So, this is an interesting usability idea.
My main concern is that more often than not no input names means user forgot to include it, rather than it being intentional. This check has saved me from making mistakes in network structure more than a few times.

Now, what might be better is a new method: something like "appendLayer(String, Layer)" Not sure on the best name for that method. But with that, it's explicit - there's no ambiguity for "no input means user forgot" vs. "no input means user wants last layer/vertex to be used".

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Yeah, that makes sense. I can't think of anything better than appendLayer.

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

What do you think about also doing it if null is passed to addVertex/Layer? There is some ambiguity in the method call as to whether you pass it as null or [null] but I can check for both.

rnett added some commits Mar 30, 2019

@AlexDBlack
Copy link
Member

left a comment

This looks good - but I don't like the "treat null as append" behaviour.
More likely than not, that will be an error (i.e., unintentional behaviour by the user). If users want to use the last added layer/vertex, they should use appendLayer only.
So let's remove that "append on null" and this can be merged 👍

rnett added some commits Apr 2, 2019

@AlexDBlack

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Not sure if you missed it or haven't got to it, but Javadoc still mentions previous null behaviour, and there's a removed test in testInvalidConfigurations() that we should keep.

rnett added some commits Apr 2, 2019

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Should both be fixed.

@AlexDBlack
Copy link
Member

left a comment

LGTM, thanks! 👍

@AlexDBlack AlexDBlack merged commit 509af15 into deeplearning4j:master Apr 2, 2019

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit is being built
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
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.