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

Arbiter enhancements #6089

Merged
merged 11 commits into from Aug 6, 2018

Conversation

@eraly
Copy link
Contributor

commented Aug 6, 2018

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

@AlexDBlack
Copy link
Contributor

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.

Copy link
@AlexDBlack

AlexDBlack Aug 6, 2018

Contributor

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.

Copy link
@eraly

eraly Aug 6, 2018

Author Contributor

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

eraly added 2 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.

Copy link
@eraly

eraly Aug 6, 2018

Author Contributor

Made this public.

@AlexDBlack
Copy link
Contributor

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.

Copy link
@AlexDBlack

AlexDBlack Aug 6, 2018

Contributor

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.

Copy link
@eraly

eraly Aug 6, 2018

Author Contributor

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
Projects
None yet
2 participants
You can’t perform that action at this time.