Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Add tf.linalg.gramSchmidt and tf.eye #1024

Merged
merged 11 commits into from
May 8, 2018
Merged

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented May 7, 2018

  • tf.linalg.gramSchmidt performs Gram-Schmidt orthogonalization on
    Matrices or arrays of vectors. It will be used to speed up
    the orthogonal initialization of weights (e.g., in RNNs).
  • tf.eye generates identity matrices.

Towards: tensorflow/tfjs#245


This change is Reviewable

* `tf.linalg.gramSchmidt` performs Gram-Schmidt orthogonalization on
  Matrices or arrays of vectors. It will be used to speed up
  the orthogonal initialization of weights (e.g., in RNNs).
* `tf.eye` generates identity matrices.

Towards: tensorflow/tfjs#245
Copy link
Contributor

@bileschi bileschi left a comment

Choose a reason for hiding this comment

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

LGTM
One nit, one food for thought re. random tests.

const ys: Tensor1D[] = [];
for (let i = 0; i < xs.length; ++i) {
ys.push(Tracking.tidy(() => {
let x = (xs as Tensor1D[])[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

xs was already cast to Tensor1D in line 62

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The linter complains if I remove this explicit casting.

describeWithFlags('gramSchmidt-tiny', ALL_ENVS, () => {
// TODO(cais): Small things should get tested on ALL_ENVS.
it('2x2, Array of Tensor1D', () => {
const xs: Tensor1D[] = [tf.tensor1d([1, 2]), tf.tensor1d([-1, 5])];
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a good candidate for a random array as input

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Using random vectors and matrics in this test file, wherever it makes sense.

@bileschi
Copy link
Contributor

bileschi commented May 8, 2018

:lgtm_strong:


Review status: 0 of 5 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@davidsoergel
Copy link
Member

:lgtm_strong:


Reviewed 3 of 5 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


src/ops/array_ops.ts, line 376 at r3 (raw file):

Nnumber

typo


src/ops/linalg_ops.ts, line 71 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

The linter complains if I remove this explicit casting.

Could avoid this by creating a local xs1d instead of overwriting the xs parameter


Comments from Reviewable

caisq added 2 commits May 8, 2018 11:00
* Use as2D() in tf.eye
* Define xs1d in gramSchmidt().
@caisq
Copy link
Collaborator Author

caisq commented May 8, 2018

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/ops/linalg_ops.ts, line 71 at r1 (raw file):

Previously, davidsoergel (David Soergel) wrote…

Could avoid this by creating a local xs1d instead of overwriting the xs parameter

Done.


Comments from Reviewable

@caisq
Copy link
Collaborator Author

caisq commented May 8, 2018

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


src/ops/array_ops.ts, line 376 at r3 (raw file):

Previously, davidsoergel (David Soergel) wrote…
Nnumber

typo

Done.


Comments from Reviewable

@dsmilkov
Copy link
Contributor

dsmilkov commented May 8, 2018

Review status: 3 of 5 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/ops/array_ops.ts, line 381 at r4 (raw file):

   */
  @operation
  static eye(n: number, dtype: DataType = 'float32'): Tensor2D {

align with tf.eye by adding numRows, numColumns and batchShape


src/ops/linalg_ops.ts, line 46 at r5 (raw file):

   */
  @operation
  static gramSchmidt(xs: Tensor1D[]|Tensor2D): Tensor1D[]|Tensor2D {

what's the tensorflow's equivalent op and can we align that API?


Comments from Reviewable

@dsmilkov
Copy link
Contributor

dsmilkov commented May 8, 2018

:lgtm_strong: Nice work! Left couple more comments, but nothing major.


Review status: 3 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


src/ops/linalg_ops.ts, line 35 at r5 (raw file):

   * @param xs The vectors to be orthogonalized, in one of the two following
   *   formats:
   *   - A non-empty `Array` of `Tensor1D`.

Just say: - an array of Tensor1D (no need to mark Array in quotes and no need to mention non-empty since its implicit)


src/ops/linalg_ops.ts, line 46 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

what's the tensorflow's equivalent op and can we align that API?

Talked offline. Resolving


src/ops/linalg_ops_test.ts, line 24 at r5 (raw file):

describeWithFlags('gramSchmidt-tiny', ALL_ENVS, () => {
  // TODO(cais): Small things should get tested on ALL_ENVS.

is this TODO outdated?


Comments from Reviewable

@caisq
Copy link
Collaborator Author

caisq commented May 8, 2018

Review status: 1 of 5 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.


src/ops/array_ops.ts, line 381 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

align with tf.eye by adding numRows, numColumns and batchShape

Done.


src/ops/linalg_ops.ts, line 35 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Just say: - an array of Tensor1D (no need to mark Array in quotes and no need to mention non-empty since its implicit)

Done.


src/ops/linalg_ops_test.ts, line 24 at r5 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

is this TODO outdated?

Yes. Removed.


Comments from Reviewable

@caisq caisq merged commit 9fb5065 into tensorflow:master May 8, 2018
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.

4 participants