Skip to content

Comments

Implement element-wise logical operations#55

Merged
huningxin merged 3 commits intowebmachinelearning:mainfrom
BruceDai:add_logical
Nov 17, 2023
Merged

Implement element-wise logical operations#55
huningxin merged 3 commits intowebmachinelearning:mainfrom
BruceDai:add_logical

Conversation

@BruceDai
Copy link
Contributor

@huningxin @fdwr PTAL, thanks.

    equal / greater / greaterOrEqual
    lesser / lesserOrEqual / not
src/logical.js Outdated
* @param {Function} logicalFunc
* @return {Tensor}
*/
function logical(inputA, inputB, logicalFunc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please take another look, thanks.

Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

Only minor readibility comments. Thanks Bruce.

for (let i = 0; i < sizeOfShape(input.shape); ++i) {
const a = input.getValueByIndex(i);
if (!Number.isInteger(a)) {
throw new Error('Invalid input value, it should be an integer in the interval [0, 255]');
Copy link

Choose a reason for hiding this comment

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

Suggested change
throw new Error('Invalid input value, it should be an integer in the interval [0, 255]');
throw new Error('Invalid input value - it should be an integer in the interval [0, 255]');

or

-      throw new Error('Invalid input value, it should be an integer in the interval [0, 255]');
+      throw new Error('Invalid input value: it should be an integer in the interval [0, 255]');

or

-      throw new Error('Invalid input value, it should be an integer in the interval [0, 255]');
+      throw new Error('Invalid input value. It should be an integer in the interval [0, 255]');

(minor grammar) An em dash or colon would be more appropriate here, as they set off extra information such as explanatory phrases or supplemental facts in a later clause that explains or expands upon on what precedes it. Commas are more for dependent clauses.

Copy link

@fdwr fdwr Nov 17, 2023

Choose a reason for hiding this comment

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

Btw, did you intend to use a distinct error message for these two cases? Otherwise we could collapse 7 lines to 3 lines:

      if (!Number.isInteger(a) || a < 0 || a > 255) {
        throw new Error('Invalid input value - it should be an integer in the interval [0, 255]');
      }

src/logical.js Outdated
Comment on lines 49 to 51
export const greaterOrEqual = (inputA, inputB) => logical(inputA, inputB, (a, b) => a >= b);
export const lesser = (inputA, inputB) => logical(inputA, inputB, (a, b) => a < b);
export const lesserOrEqual = (inputA, inputB) => logical(inputA, inputB, (a, b) => a <= b);
Copy link

Choose a reason for hiding this comment

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

👀🔍 It's visually a little challenging to parse these two parts:

(a, b) => a >= b
(a, b) => a <= b

Considering wrapping them for increased readability:

(a, b) => (a >= b)
(a, b) => (a <= b)

Copy link

@fdwr fdwr left a comment

Choose a reason for hiding this comment

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

👍😎

Copy link
Contributor

@huningxin huningxin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants