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

Nd4j.java javadoc #7997

Merged
merged 10 commits into from Jul 20, 2019

Conversation

@RobAltena
Copy link
Contributor

commented Jul 9, 2019

Refactoring of the javadoc in Nd4j.java.

All issues that we stumble upon during the refactor are flagged with TODO comments to be taken down in other pull requests.

RobAltena added 10 commits Jul 9, 2019
up to line 500.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 1120.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to 1286.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 2400.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 2500.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 3600.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 4000.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 4500.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 5000.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>
up to line 6000.
Signed-off-by: Robert Altena <Rob@Ra-ai.com>

@RobAltena RobAltena marked this pull request as ready for review Jul 19, 2019

@AlexDBlack
Copy link
Contributor

left a comment

overall this is great, a huge step forward.
And good catch on those ones to be reviewed/removed, we'll do a pass on those later.

Most of it is good - but there's 4 things here to improve:

  1. When argument differ, specify the default behaviour.
    For example: x(int a) calls x(int a, int b), and the former has a javadoc see/link - what is the value of b?
    Thes "default values" must be included in the javadoc, otherwise the user doesn't know what the actual behaviour is, if all they have to go by is the javadoc for the "full" version of the method.

  2. Formatting. See Nd4j.sort comment for a good example.

  3. Null behaviour. When null is supported, we should say so in the javadoc - and identify what the behaviour is, given null inputs.
    If null is NOT allowed, then adding a lombok @NonNull annotation is kind of self documenting, it ends up in the javadoc: ctrl+f @NonNull here: https://deeplearning4j.org/api/latest/org/deeplearning4j/nn/multilayer/MultiLayerNetwork.html

  4. Deprecations - see Nd4j.writeTxtString comment

Also as noted when we talked, I'll merge this now (due to fork/master alignment process) and we'll fix this up in a later PR.

int slices = arrays.length;
INDArray ret = Nd4j.create(ArrayUtil.combine(new int[] {slices}, sliceShape));
for (int i = 0; i < ret.slices(); i++)
ret.putSlice(i, Nd4j.create(arrays[i]).reshape(ArrayUtil.toLongArray(sliceShape)));
return ret;
}

/**
* @see #create(LongShapeDescriptor)
*/

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

Self referential. Should be #create(LongShapeDescriptor, boolean).
Also as per above: mention the value of the other arg ("with initialize = true (values set to 0)")

This comment has been minimized.

Copy link
@RobAltena
* @param arr
* @param dimension
* @return
* Get the maximum (argmax) or minimum (argmin) values for a dimension.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

I get what you're going for here. But suppose the user comes directly to the argMax method (not via the argMin see/link).
They could reasonably intepret this to mean that somehow the minimum might be returned by this method.

Better: only talk about argmax here. And for argMin, do something like An per {@link argMax(...)} but for minimum values

This comment has been minimized.

Copy link
@RobAltena
public static NDArrayFactory sparseFactory() {
return SPARSE_INSTANCE;
}

// TODO: We forward the following methods with a default dimension argument. Doc should say something about that.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

Integer.MAX_VALUE is full array reduction.
So I'd go with something like: Full array max operation, returns scalar NDArray. See: {@link ...}

This comment has been minimized.

Copy link
@RobAltena

RobAltena Jul 22, 2019

Author Contributor

Updating just the cumsum docs in the next PR. Will keep the todo and expand to all methods once it passes review.

This comment has been minimized.

Copy link
@RobAltena
* @param shape the shape of the buffer to create
* @param type the opType to create
* @return
*/

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

Clarify: "Detached" here means "Not in any memory workspace, even if a workspace is currently open".

This comment has been minimized.

Copy link
@RobAltena
* @deprecated precision can no longer be specified. The array is written in scientific notation.
* Use {@link #writeTxtString(INDArray, OutputStream)}
* @see #writeTxtString(INDArray, OutputStream)
*/
public static void writeTxtString(INDArray write, OutputStream os, String split, int precision) {

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

Technically not javadoc, but you might as well fix them when you see them.
Deprecated methods should follow the following format (usually)

  1. An @Deprecated annotation on the method (or class if applicable)
  2. A @deprecated javadoc tag
  3. Javadoc tag should include what to use instead (+ why it was deprecated, if applicable)
  4. Usually - no other comments other than the deprecated information (to discourage users from using it at all)

Also a minor nitpick: I prefer the {@link ...} style directly in the @deprecated tag.
@see gets rendered to javadoc as something like:

See Also:
someMethod(int, double)

Semantically - "see also" is different to "use this instead".

* @param dataType datatype
* @param shape shape
* @return new array.
*/

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

Random methods like this must all specify the distribution.
"Uniformly distributed in the range 0.0 to 1.0" in this case.

* @param dataType datatype to use, must be a float type datatype.
* @param shape shape for the new array.
* @return new array with random values
*/

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

"Random normal ndarray " -> "Random normal (Gaussian N(0,1)) with mean 0 and standard deviation 1"

* @return the created ndarray
* Create an array withgiven shape and ordering based on a java double array.
* @param data java array used for initialisation. Must have at least the number of elements required.
* @@param shape desired shape of new array.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

@@

*/
public static INDArray zeros(int columns) {
return INSTANCE.zeros(columns);
}

/**
* Creates a 1D array with the specified data tyoe and number of columns initialized with zero.

This comment has been minimized.

Copy link
@AlexDBlack

AlexDBlack Jul 20, 2019

Contributor

specified data tyoe
number of columns initialized with zero. -> rephrase, sounds like number of columns will be initialized to zero.
"number of columns, with values initialized to zero"

@AlexDBlack AlexDBlack merged commit 0ef373f into eclipse:master Jul 20, 2019

1 check passed

eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.