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

TF import tests - adding missing operations #65

Merged
merged 33 commits into from Dec 2, 2019
Merged

Conversation

@alexanderst
Copy link
Member

alexanderst commented Nov 20, 2019

Adding missing operations to ND4J.
Fixing import and TF binding related issues.

@alexanderst alexanderst changed the title TF import tests - adding missing operations [WIP] TF import tests - adding missing operations Nov 21, 2019
@alexanderst alexanderst marked this pull request as ready for review Nov 22, 2019
Copy link
Member

AlexDBlack left a comment

Overall structure is good. 👍
Other than MaxPoolWithArgMax, it's just a bunch of minor things that need fixing and polish.

We'll also want TF import tests for these (both to check import correctness, as well as calculateOutputDataTypes which will be used whenever a graph is defined with the op in it)

*/
public SDVariable[] maxPoolWithArgmaxs(String name, SDVariable x) {
SDVariable[] res = f().maxPoolWithArgmaxs(x);
return res;

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

missing updateVariableNameAndReference
Because there are 2 outputs, let's use this:

if(name != null){
    updateVariableNameAndReference(res[0], name + "/output");
    updateVariableNameAndReference(res[1], name + "/index");
}

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 27, 2019

Member

Also @NonNull and javadoc tag for pooling2DConfig arg

@@ -14,11 +15,11 @@

public AdjustContrast() {super();}

public AdjustContrast(INDArray in, double factor, INDArray out) {
public AdjustContrast(@NonNull INDArray in, double factor, @NonNull INDArray out) {

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

Adding @NonNull is good in general
but output arrays can be null (inputs must be non-null, however).
Null output means "automatically allocate an output array during execution"
So keep NonNull on in, and remove it from out

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 22, 2019

Author Member

Done.

@@ -14,11 +15,11 @@

public AdjustContrastV2() {super();}

public AdjustContrastV2(INDArray in, double factor, INDArray out) {
public AdjustContrastV2(@NonNull INDArray in, double factor, @NonNull INDArray out) {

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

As per AdjustContrast

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 22, 2019

Author Member

Done


public ToggleBits(@NonNull INDArray input, @NonNull INDArray output) {
this(input);
addOutputArgument(input);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

Output may be null

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 28, 2019

Member

Null check for output - and incorrectly passing input array here, not output array

*/
public static INDArray copyDeep(INDArray ndArray) {
return Nd4j.getExecutioner().exec(new Identity(ndArray, ndArray.ulike()))[0];
}

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

We don't need these methods, for 2 reasons
(a) Transforms class is going away, we'll use codegen to generate the ND4J equivalents of SDMath, SDNN, etc
(b) Copy, CopyHost and CopyDeep are needed for TF import only - I don't want them in the SameDiff or ND4J public API like this... users can use INDArray.dup() to get exactly this

import org.nd4j.linalg.api.ndarray.INDArray;
import org.nd4j.linalg.api.ops.DynamicCustomOp;

public class MaxPoolWithArgmax extends DynamicCustomOp {

This comment has been minimized.

Copy link
@AlexDBlack
INDArray[] res = Nd4j.exec(op);

assertEquals(x, res[0]);
assertEquals(expected, res[1]);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 22, 2019

Member

Given the lack of arguments noted earlier, I can't see how this is passing.

public SDVariable[] fusedBatchNorm(String name, SDVariable x, SDVariable scale, SDVariable offset,
SDVariable dataFormat, SDVariable isTraining) {
SDVariable[] res = f().fusedBatchNorm(x,scale,offset,dataFormat,isTraining);
return res;

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 27, 2019

Member

Missing updateVariableNameAndReference, @NonNull. Name arg javadoc should mention it can be null.

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 27, 2019

Author Member

Done.

public BetaInc(@NonNull INDArray a_input, @NonNull INDArray b_input, @NonNull INDArray x_input,
INDArray output) {
addInputArgument(a_input, b_input, x_input);
addOutputArgument(output);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 27, 2019

Member

Null check on the output argument - we don't want to call addOutputArgument(null)
There's a number of other ops like that, that also need to be fixed

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 27, 2019

Author Member

So we need ability to pass output == null, but don't call addOutputArguments(), right? So we need if (output != null) and @nonnull will not work.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 28, 2019

Member

Correct. No @NonNull annotation on the output arg, and then just:

if(output != null){
    addOutputArgument(output);
}

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 28, 2019

Author Member

Done.

Preconditions.checkState(inputDataTypes != null && inputDataTypes.size() == 1, "Expected 1 input data type for %s, got %s", getClass(), inputDataTypes);
List<DataType> result = new ArrayList<>();
result.add(inputDataTypes.get(0));
result.add(DataType.LONG);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 27, 2019

Member

TF also allows output datatype to be configured - INT32 or INT64

This comment has been minimized.

Copy link
@alexanderst

alexanderst Nov 28, 2019

Author Member

Done

Copy link
Member

AlexDBlack left a comment

Mostly good, few small fixes still required

@NonNull INDArray yOut, @NonNull INDArray batchMeanOut, @NonNull INDArray batchMeanVar) {
addInputArgument(x, scale, offset);
addIArgument(dataFormat, isTraining);
addOutputArgument(yOut, batchMeanOut, batchMeanVar);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 28, 2019

Member

Null check on output arguments

Preconditions.checkArgument(minLower > -N && minLower < N, "MatrixBandPart: lower diagonal count %i should be less than %i",
minLower, N);
Preconditions.checkArgument(maxUpper > -M && maxUpper < M, "MatrixBandPart: upper diagonal count %i should be less than %i.",
maxUpper, M);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 28, 2019

Member

Still have %i here, this needs to be fixed (replace with %s)


public ToggleBits(@NonNull INDArray input, @NonNull INDArray output) {
this(input);
addOutputArgument(input);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Nov 28, 2019

Member

Null check for output - and incorrectly passing input array here, not output array

Copy link
Member

AlexDBlack left a comment

LGTM 👍

Edit: though there's some conflicts that need to be resolved first.

@alexanderst alexanderst changed the title [WIP] TF import tests - adding missing operations TF import tests - adding missing operations Nov 29, 2019
alexanderst and others added 5 commits Nov 29, 2019
Signed-off-by: raver119 <raver119@gmail.com>
Copy link
Member

AlexDBlack left a comment

LGTM 👍

@AlexDBlack AlexDBlack merged commit 5e152c0 into master Dec 2, 2019
1 check was pending
1 check was pending
continuous-integration/jenkins/pr-head This commit is being built
Details
@AlexDBlack AlexDBlack deleted the asto_20191118_tf_import branch Dec 2, 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.