-
Notifications
You must be signed in to change notification settings - Fork 950
Conversation
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 :) |
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. |
There was a problem hiding this 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.
src/ops/logical_ops.ts
Outdated
@@ -27,6 +27,12 @@ export class LogicalOps { | |||
/** | |||
* Returns the truth value of `NOT x` element-wise. | |||
* | |||
* ```js | |||
* var a = dl.tensor1d([0,1],'bool') |
There was a problem hiding this comment.
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
src/ops/logical_ops.ts
Outdated
* ```js | ||
* var a = dl.tensor1d([0,1],'bool') | ||
* | ||
* dl.logicalNot(a) |
There was a problem hiding this comment.
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()
src/ops/logical_ops.ts
Outdated
@@ -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') |
There was a problem hiding this comment.
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.
src/ops/logical_ops.ts
Outdated
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
src/ops/logical_ops.ts
Outdated
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
src/ops/logical_ops.ts
Outdated
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here as well.
src/ops/logical_ops.ts
Outdated
* var a = dl.tensor1d([1,2,3]) | ||
* var b = dl.tensor1d([-1,-2,-3]) | ||
* | ||
* dl.where(cond,a,b).print() |
There was a problem hiding this comment.
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()
Everything should be ready: using `tf`, `const`, and chaining.
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):
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):
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 |
@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. |
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…
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 |
@dsmilkov Should be good to go. |
Reviewed 1 of 2 files at r4. Comments from Reviewable |
Thank you!! |
This change is