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

Add logic operation examples #900

Merged
merged 9 commits into from
Apr 24, 2018
Merged

Add logic operation examples #900

merged 9 commits into from
Apr 24, 2018

Conversation

nbardy
Copy link
Contributor

@nbardy nbardy commented Mar 26, 2018

This change is Reviewable

@nsthorat
Copy link
Contributor

Thanks for these awesome contributions! Unfortunately we're on a small code freeze until Friday, but as soon as that's over I'll merge these in :)

@nbardy
Copy link
Contributor Author

nbardy commented Mar 27, 2018

Thanks to you as well. I've been really enjoying the library! First time working with ML and the documentation(especially the examples) has been really useful.

Copy link
Contributor

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

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

Thanks @nbardy for the PR! Looks great! :) I've left few comments that might need attention before this can be merged.

@@ -27,6 +27,12 @@ export class LogicalOps {
/**
* Returns the truth value of `NOT x` element-wise.
*
* ```js
* var a = dl.tensor1d([0,1],'bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this const a = tf.tensor1d([0, 1], 'bool') with proper formatting

* ```js
* var a = dl.tensor1d([0,1],'bool')
*
* dl.logicalNot(a)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a.logicalNot().print()

@@ -39,6 +45,13 @@ export class LogicalOps {
/**
* Returns the truth value of a AND b element-wise. Supports broadcasting.
*
* ```js
* var a = dl.tensor1d([0,0,1,1],'bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's change var to const and dl to tf with formatting as above.

@@ -56,6 +69,12 @@ export class LogicalOps {
/**
* Returns the truth value of `a OR b` element-wise. Supports broadcasting.
*
* ```js
* var a = dl.tensor1d([0,0,1,1],'bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@@ -73,6 +92,13 @@ export class LogicalOps {
/**
* Returns the truth value of `a XOR b` element-wise. Supports broadcasting.
*
* ```js
* var a = dl.tensor1d([0,0,1,1],'bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@@ -92,6 +118,14 @@ export class LogicalOps {
*
* If the condition is true, select from `a`, otherwise select from `b`.
*
* ```js
* var cond = dl.tensor1d([0,0,1],'bool')
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as well.

* var a = dl.tensor1d([1,2,3])
* var b = dl.tensor1d([-1,-2,-3])
*
* dl.where(cond,a,b).print()
Copy link
Contributor

Choose a reason for hiding this comment

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

We can chain this to a.where(cond, b).print()

nbardy added 2 commits April 2, 2018 15:25
Everything should be ready: using `tf`, `const`, and chaining.
@nsthorat
Copy link
Contributor

nsthorat commented Apr 7, 2018

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


src/ops/logical_ops.ts, line 31 at r2 (raw file):

   *
   * ```js
   * const a = tf.tensor1d([0, 1], 'bool');

you can use true and false as values to the bool, it might make this clearer


src/ops/logical_ops.ts, line 33 at r2 (raw file):

   * const a = tf.tensor1d([0, 1], 'bool');
   *
   * a.logicalNot().print();

it looks like we don't actually have this on the chain API, this is a mistake!

If you're up for it, could you add a method like this to tensor.ts?

/~https://github.com/tensorflow/tfjs-core/blob/master/src/tensor.ts#L666


Comments from Reviewable

@nbardy
Copy link
Contributor Author

nbardy commented Apr 10, 2018

@nsthorat I can write the method in a bit. Having local dev environment issues at the moment so I'll get to it some point this week.

@dsmilkov
Copy link
Contributor

Review status: 0 of 2 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


src/ops/logical_ops.ts, line 31 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you can use true and false as values to the bool, it might make this clearer

I think the only comment left is to replace 0 and 1 with false and true to be clearer. Let me know when you do that and we'll merge this in. Thank you!


Comments from Reviewable

@nbardy
Copy link
Contributor Author

nbardy commented Apr 24, 2018

@dsmilkov Should be good to go.

@dsmilkov
Copy link
Contributor

:lgtm_strong:


Reviewed 1 of 2 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.


Comments from Reviewable

@dsmilkov
Copy link
Contributor

Thank you!!

@dsmilkov dsmilkov merged commit ce93412 into tensorflow:master Apr 24, 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