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

[WIP] SameDiff TF import #49

Merged
merged 33 commits into from Nov 19, 2019
Merged

[WIP] SameDiff TF import #49

merged 33 commits into from Nov 19, 2019

Conversation

@AlexDBlack
Copy link
Member

AlexDBlack commented Nov 15, 2019

No description provided.

shugeo and others added 28 commits Nov 6, 2019
…rts with the test name

Signed-off-by: AlexDBlack <blacka101@gmail.com>
Signed-off-by: AlexDBlack <blacka101@gmail.com>
Signed-off-by: AlexDBlack <blacka101@gmail.com>
Signed-off-by: AlexDBlack <blacka101@gmail.com>
@alexanderst alexanderst marked this pull request as ready for review Nov 18, 2019
Copy link
Member Author

AlexDBlack left a comment

Overall LGTM, few minor issues to fix.
Main thing is a cleanup for tensorflowName() / onnxName() methods (we can just delete them).

After that, it can be merged 👍

@@ -67,7 +68,8 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "BiasAddGrad";
throw new NoOpNameFoundException("TF mapping is skipped for " + opName());
//return "BiasAddGrad";
}

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 18, 2019

Author Member

We can just delete the whole tensorflowName method here, DynamicCustomOp tensorflowName() method throws an exception already.
Same thing for all the other classes like this.

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 18, 2019

Member

Fixed for DynamicCustomOp children.

iArguments.clear();
tArguments.clear();
//iArguments.clear();
//tArguments.clear();

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 18, 2019

Author Member

Checking code, it should be safe to remove this. Let's actually delete it then :)

This comment has been minimized.

Copy link
@alexanderst
@@ -65,7 +65,7 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "less";
return "Less";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 18, 2019

Author Member

This mapping is incorrect, and already mapped to LessThan custom op - remove method.

This comment has been minimized.

Copy link
@alexanderst
alexanderst added 4 commits Nov 18, 2019
Copy link
Member Author

AlexDBlack left a comment

There's still a whole lot of tensorflowName methods that should be removed. To do that for these methods (that don't extend DynamicCustomOp), we'll need a tensorflowName method (that throws an exception) in BaseOp. Also an onnxName() method there too.

@@ -233,7 +234,8 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "Pooling2D";
throw new NoOpNameFoundException("Tensorflow name not found for " + opName());
//return "Pooling2D";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 19, 2019

Author Member

Remove

This comment has been minimized.

Copy link
@alexanderst
@@ -66,7 +66,8 @@ public String opName() {

@Override
public String tensorflowName() {
return "sigmoid_cross_entropy";
throw new NoOpNameFoundException("No tensorflow name found for " + opName());
//return "sigmoid_cross_entropy";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 19, 2019

Author Member

Remove

This comment has been minimized.

Copy link
@alexanderst
@@ -66,7 +67,8 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "HasInf";
throw new NoOpNameFoundException("No tensorflow name found for " + opName());
//return "HasInf";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 19, 2019

Author Member

Let's remove this (and other reduce bool ops here) and instead put a method with exception in BaseOp - for tensorflowName() and onnxName() - same as DynamicCustomOp

This comment has been minimized.

Copy link
@alexanderst
@@ -64,7 +64,8 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "entropy";
throw new NoOpNameFoundException("No tensorflow op opName found for " + opName());
//return "entropy";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 19, 2019

Author Member

As per the reduce bool ops - move to BaseOp

@@ -65,7 +65,7 @@ public String onnxName() {

@Override
public String tensorflowName() {
return "less";
return "LessThan";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 19, 2019

Author Member

This is still wrong.
Previous comment:

This mapping is incorrect, and already mapped to LessThan custom op - remove method.

By this I meant that ScalarLessThan should not have a tensorflowName() method at all.
The previous "Less" TF name was incorrect, and was already imported in the ND4J LessThan method: https://github.com/eclipse/deeplearning4j/blob/master/nd4j/nd4j-backends/nd4j-api-parent/nd4j-api/src/main/java/org/nd4j/linalg/api/ops/impl/transforms/custom/LessThan.java#L66

So to be clear, delete this tensorflowName() method for the ScalarLessThan op.

This comment has been minimized.

Copy link
@alexanderst
@AlexDBlack AlexDBlack merged commit da1944e into master Nov 19, 2019
@AlexDBlack AlexDBlack deleted the asto_samediff_coverage branch Nov 19, 2019
@alexanderst alexanderst changed the title SameDiff TF import [WIP] SameDiff TF import Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.