Skip to content
This repository has been archived by the owner on Oct 17, 2021. It is now read-only.

VarianceScaling initializer: Fix default arg values #505

Merged
merged 4 commits into from
Mar 26, 2019

Conversation

caisq
Copy link
Contributor

@caisq caisq commented Mar 23, 2019

  • The VarianceScaling initializer is the base class of initializers
    such as GlorotNormal, HeNormal and LeCunNormal. But in relatively
    rare occasions, it is used by itself.
  • Previously, the default values of the mode and distribution
    were not set properly, leading to inconsistent behavior with keras.
    This CL fixes that.

Re tensorflow/tfjs#1434


This change is Reviewable

- The VarianceScaling initializer is the base class of initializers
  such as GlorotNormal, HeNormal and LeCunNormal. But in relatively
  rare occasions, it is used by itself.
- Previously, the default values of the `mode` and `distribution`
  were not set properly, leading to inconsistent behavior with keras.
  This CL fixes that.
@caisq caisq requested a review from davidsoergel March 25, 2019 20:35
Copy link
Member

@davidsoergel davidsoergel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @davidsoergel)


src/initializers.ts, line 329 at r1 (raw file):

  /** Fanning mode for inputs and outputs. */
  mode: FanMode;

Should these be mode? and distribution? now that you provide defaults?

And, assuming yes, can you update the keras_format VarianceScalingConfig also?

Copy link
Contributor Author

@caisq caisq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @davidsoergel)


src/initializers.ts, line 329 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Should these be mode? and distribution? now that you provide defaults?

And, assuming yes, can you update the keras_format VarianceScalingConfig also?

Yep. Done.

Copy link
Member

@davidsoergel davidsoergel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @davidsoergel)

@caisq caisq merged commit fd00f6c into tensorflow:master Mar 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants