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

Arbiter enhancements #6089

Merged
merged 11 commits into from Aug 6, 2018

Conversation

Projects
None yet
2 participants
@eraly
Copy link
Member

eraly commented Aug 6, 2018

@eraly eraly requested a review from AlexDBlack Aug 6, 2018

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

LGTM, only real issue here is dilation as noted it comment.

import org.deeplearning4j.nn.conf.layers.Deconvolution2D;

@Data
@EqualsAndHashCode(callSuper = true)
@NoArgsConstructor(access = AccessLevel.PROTECTED) //For Jackson JSON/YAML deserialization
public class Deconvolution2DLayerSpace extends BaseConvolutionLayerSpace<Deconvolution2D> {
protected ParameterSpace<int[]> dilation;

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 6, 2018

Member

Move this (and all dilation-related functionality - setLayerOptionsBuilder etc) to BaseConvolutionLayerSpace, it applies to all conv layers.

Though looks like we still need it again in SubsamplingLayerSpace... that doesn't extend BaseConvolutionLayerSpace for some reason. Let's not refactor it now though...

This comment has been minimized.

@eraly

eraly Aug 6, 2018

Member

ConvolutionLayerSpace also doesn't extend BaseConvolutionLayerSpace. Fix in next release.

eraly added some commits Aug 6, 2018

@@ -324,14 +331,14 @@ public ConvolutionLayer build() {
}
}

protected static abstract class BaseConvBuilder<T extends BaseConvBuilder<T>> extends FeedForwardLayer.Builder<T> {
public static abstract class BaseConvBuilder<T extends BaseConvBuilder<T>> extends FeedForwardLayer.Builder<T> {

This comment has been minimized.

@eraly

eraly Aug 6, 2018

Member

Made this public.

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

One minor change required

@@ -55,7 +55,7 @@ protected BaseConvolutionLayerSpace(Builder builder) {
this.numParameters = LeafUtils.countUniqueParameters(collectLeaves());
}

protected void setLayerOptionsBuilder(ConvolutionLayer.Builder builder, double[] values) {
protected void setLayerOptionsBuilder(Deconvolution2D.Builder builder, double[] values) {

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 6, 2018

Member

Change this to setLayerOptionsBuilder(Builder builder, double[] values)

this.dilation = dilation;
return (T) this;
}

/** Defaults to "PREFER_FASTEST", but "NO_WORKSPACE" uses less memory. */
public T kernelSize(int... kernelSize) {

This comment has been minimized.

@eraly

eraly Aug 6, 2018

Member

Needed to add these to use in arbiter.

@AlexDBlack AlexDBlack merged commit 0068413 into master Aug 6, 2018

1 of 2 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@AlexDBlack AlexDBlack deleted the earlybird branch Aug 6, 2018

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