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

ND4J: array creation methods should validate array order arg #6442

Closed
Charele opened this Issue Sep 14, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@Charele
Copy link

Charele commented Sep 14, 2018

We know we have a order option('f','c') when operating INDArray, it's related with internal storage.
I find I can use any char,,,
val shape = Array(5, 3)
val a1 = Nd4j.rand('c', shape)
val a2 = Nd4j.rand('f', shape)
val a3 = Nd4j.rand('x', shape)

println(a1.shapeInfoToString())
println(a2.shapeInfoToString())
println(a3.shapeInfoToString())

Rank: 2,Offset: 0
Order: c Shape: [5,3], stride: [3,1]
Rank: 2,Offset: 0
Order: f Shape: [5,3], stride: [1,5]
Rank: 2,Offset: 0
Order: x Shape: [5,3], stride: [3,1]

I'm so chaotic until I see it:
public static int[] getStrides(int[] shape, char order) {
if (order == NDArrayFactory.FORTRAN)
return ArrayUtil.calcStridesFortran(shape);
return ArrayUtil.calcStrides(shape);
}

My question is, why don't restrict the character in API?

PS: I see some code in Shape.java
...
} else if (order == 'a') {
return true;
} else {
throw new RuntimeException("Invalid order: not c or f (is: " + order + ")");
}
Is the 'a' another special option?

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Sep 14, 2018

So it's just a lack of validation then. The only valid values for that method (and those like it) is 'c' or 'f'.
Feel free to send us a pull request to add that validation.

@Charele

This comment has been minimized.

Copy link

Charele commented Sep 14, 2018

Hi Alex, You mean I send this topic to "Pull Request" box again?

I have another doubt(only my doubt,not bug/error), In LSTMHelpers.java, activateHelper() method

qq

Is the "training" necessary?
My understand, when "forBackprop" is true, I think "training" must be true,is it?

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Sep 14, 2018

Hi Alex, You mean I send this topic to "Pull Request" box again?

No, I mean you can - if you want - implement a fix and submit it via a pull requests: https://help.github.com/articles/about-pull-requests/

My understand, when "forBackprop" is true, I think "training" must be true,is it?

Backprop == true implies training == true, yes. It's not a big deal, but it could be cleaned up.
FYI The converse doesn't always hold though: i.e., training==true but backprop == false - the API allows for 'train' flag to be set to true outside of training. That can be useful in some cases.

@Charele

This comment has been minimized.

Copy link

Charele commented Sep 14, 2018

THANKS:-O

@AlexDBlack AlexDBlack changed the title About order in Nd4j ND4J: array creation methods should validate array order arg Sep 17, 2018

AlexDBlack added a commit that referenced this issue Sep 17, 2018

AlexDBlack added a commit that referenced this issue Sep 18, 2018

Various fixes (#6450)
* #6401 transposei fix

* #6378 MmulTranspose fix + cleanup

* #6442 validate array order

* Cleanup and test fixes

* #6403 batch norm validation

* #6389 Fix TransferLearning nOutReplace issues; add nInReplace method

* Trigger CI
@lock

This comment has been minimized.

Copy link

lock bot commented Oct 18, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Oct 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.