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] Size of empty INDArray fixed #7163

Merged
merged 36 commits into from Feb 20, 2019

Conversation

Projects
None yet
3 participants
@alexanderst
Copy link
Contributor

commented Feb 13, 2019

What changes were proposed in this pull request?

Empty array will have 0 size now.
JvmShapeInfo becomes immutable.

How was this patch tested?

Unit testing.

Quick checklist

The following checklist helps ensure your PR is complete:

  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
  • Ran mvn formatter:format (see formatter instructions for targeting your specific files).

@raver119 raver119 changed the title Size of empty INDArray fixed [WIP] Size of empty INDArray fixed Feb 13, 2019

alexanderst and others added some commits Feb 13, 2019

Merge remote-tracking branch 'origin/asto_ND4J_bugfixes' into asto_ND…
…4J_bugfixes

# Conflicts:
#	nd4j/nd4j-backends/nd4j-tests/src/test/java/org/nd4j/linalg/Nd4jTestsC.java

alexanderst and others added some commits Feb 19, 2019

@AlexDBlack
Copy link
Member

left a comment

Overall good, mainly reviewing for DL4J changes (which are fine). 👍
I did note a small number of minor issues, however.

@@ -59,6 +59,7 @@
public class Conv1D extends DynamicCustomOp {

protected Conv1DConfig config;
private static final String INVALID_CONFIGURATION = "Invalid Conv1D configuration : s = %d p = %d ";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

Minor issue: Should always be %s - Preconditions doesn't need format specified for different types.
Everything eventually ends up using Object.toString() anyway for these.
Only valid format specifiers other than %s are these: https://github.com/deeplearning4j/deeplearning4j/blob/master/nd4j/nd4j-backends/nd4j-api-parent/nd4j-api/src/main/java/org/nd4j/linalg/util/NDArrayPreconditionsFormat.java#L24

This comment has been minimized.

Copy link
@alexanderst

alexanderst Feb 20, 2019

Author Contributor

There are primitive data types in context of this message, no Objects. Should I wrap long to Long?

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

No, no need to box primitives to Number classes.
That happens automatically - but only if an exception is to be thrown.
https://github.com/deeplearning4j/deeplearning4j/blob/master/nd4j/nd4j-common/src/main/java/org/nd4j/base/Preconditions.java#L73

public static void throwEx(String message, Object... args) {

The idea with all of the (seeminly redundant) primitive overloads in Preconditions class is to avoid creating objects unless an exception is thrown as much as possible, to avoid unnecessary garbage generation.

@@ -54,6 +54,7 @@
public class Conv2D extends DynamicCustomOp {

protected Conv2DConfig config;
private static final String INVALID_CONFIGURATION = "Invalid Conv2D configuration : sW = %d pH = %d dW = %d ";

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

As per previous %s comment

@@ -49,6 +49,7 @@
public class Conv3D extends DynamicCustomOp {

protected Conv3DConfig config;
private static final String INVALID_CONFIGURATION = "Invalid Conv3D configuration : sW = %d pH = %d dW = %d ";

This comment has been minimized.

Copy link
@AlexDBlack
File tmpFile = DL4JFileUtils.createTempFile("word2vec"+System.currentTimeMillis(), "tmp");
tmpFile.deleteOnExit();
FileUtils.copyInputStreamToFile(inputStream, tmpFile);
return loadStaticModel(tmpFile);

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

Better IMO to use:

try{
    return loadStaticModel(tmpFile);
} finally {
   tmpFile.delete();
}

This is better for long-lived JVMs (and those that don't exit properly).

//edge case for scalars
INDArray ret = Nd4j.createUninitialized(dtype, new long[]{data.length});
if (ret.isScalar())
return ret;

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

This looks wrong to me... No data is put for scalars - array content (single value) remains uninitialized.

@@ -197,7 +197,32 @@ public INDArray linspace(int lower, int upper, int num, DataType dtype) {
for (int i = 0; i < num; i++) {
double t = (double) i / (num - 1);
data[i] = lower * (1 - t) + t * upper;
}

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

Can we replace these BaseNDArrayFactory methods with a native op call?
Doesn't really make sense to populate in java IMO, especially for CUDA.

This comment has been minimized.

Copy link
@raver119

raver119 Feb 20, 2019

Contributor

It shouldn't be used anywhere now i think? I'd expect ops to be used for linspace everywhere from now on

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Feb 20, 2019

Member

Hm... maybe we remove linspace methods from BaseNDArrayFactory interface entirely then?

This comment has been minimized.

Copy link
@alexanderst

alexanderst Feb 20, 2019

Author Contributor

That's what I'm doing, no need for Java linspace implementation anymore.

This comment has been minimized.

Copy link
@alexanderst

alexanderst Feb 20, 2019

Author Contributor

Fixes for all issues were pushed.

@raver119 raver119 merged commit 0efea0d into master Feb 20, 2019

0 of 2 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details

@raver119 raver119 deleted the asto_ND4J_bugfixes branch Feb 20, 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.