Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create threshold.ts #4906

Merged
merged 72 commits into from Apr 23, 2021
Merged

Create threshold.ts #4906

merged 72 commits into from Apr 23, 2021

Conversation

BadMachine
Copy link
Contributor

@BadMachine BadMachine commented Apr 6, 2021

Image processing operation threshold with few methods.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

Image processing operation threshold with few methods
@google-cla
Copy link

google-cla bot commented Apr 6, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Apr 6, 2021
@google-cla google-cla bot added cla: yes and removed cla: no labels Apr 6, 2021
@BadMachine
Copy link
Contributor Author

@googlebot I signed it!

@pyu10055 pyu10055 requested a review from lina128 April 7, 2021 16:19
Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, @BadMachine! I left some comments, see below.

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


tfjs-core/src/ops/image/threshold.ts, line 1 at r4 (raw file):

import { Tensor3D } from '../../tensor';

Add license.

/**
 * @license
 * Copyright 2021 Google LLC. All Rights Reserved.
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 * =============================================================================
 */

tfjs-core/src/ops/image/threshold.ts, line 9 at r4 (raw file):

/**
 * Performs threshold algorithms on Tensors

Sugg: Performs image thresholding, which creates a binary image from a grayscale image.


tfjs-core/src/ops/image/threshold.ts, line 10 at r4 (raw file):

/**
 * Performs threshold algorithms on Tensors
 * @param image a 2d image tensor of shape `[x , y, 3]`.

Sugg: 3d tensor of shape [batch, imageHeight, imageWidth, depth]. The image color range should be [0, 255].


tfjs-core/src/ops/image/threshold.ts, line 12 at r4 (raw file):

 * @param image a 2d image tensor of shape `[x , y, 3]`.
 * @param method Optional string from `'binary' | 'otsu'`,
 *     defaults to binary, which specifies type of the threshold operation

Sugg: which specifies the method for thresholding.


tfjs-core/src/ops/image/threshold.ts, line 13 at r4 (raw file):

 * @param method Optional string from `'binary' | 'otsu'`,
 *     defaults to binary, which specifies type of the threshold operation
 * @param coeff Optional number which defines thresh coefficient from 0 to 1.

Is there a default?


tfjs-core/src/ops/image/threshold.ts, line 14 at r4 (raw file):

 *     defaults to binary, which specifies type of the threshold operation
 * @param coeff Optional number which defines thresh coefficient from 0 to 1.
 * @param inverted Optional boolian which

boolean, which specifies if color should be inverted. Defaults to false.


tfjs-core/src/ops/image/threshold.ts, line 56 at r4 (raw file):

}

function threshold_(

Move the doccomment next to this method.


tfjs-core/src/ops/image/threshold.ts, line 59 at r4 (raw file):

  image: Tensor3D | TensorLike,
  method: 'binary' | 'otsu' = 'binary',
  coeff?: number,

What's the default?


tfjs-core/src/ops/image/threshold.ts, line 60 at r4 (raw file):

  method: 'binary' | 'otsu' = 'binary',
  coeff?: number,
  inverted?: boolean

What's the default?


tfjs-core/src/ops/image/threshold.ts, line 63 at r4 (raw file):

): Tensor3D {
    
  const $image = convertToTensor(image, 'image', 'threshold');

Do you also need to check dtype?


tfjs-core/src/ops/image/threshold.ts, line 65 at r4 (raw file):

  const $image = convertToTensor(image, 'image', 'threshold');
  const threshold = coeff * 255;
  const arrayedImage = Array.from($image.dataSync());

This is a blocking call. Why not use tfjs ops? See suggestion on how to use it below.


tfjs-core/src/ops/image/threshold.ts, line 74 at r4 (raw file):

  util.assert(
    method === 'otsu' || method === 'binary',
    () => `method must be binary or otsu, but was ${method}`);

Capital M.


tfjs-core/src/ops/image/threshold.ts, line 76 at r4 (raw file):

    () => `method must be binary or otsu, but was ${method}`);

  if (method === 'binary') {
const condition = inverted ? less($image, threshold) : greater($image, threshold);
const result = where(condition, 255, 0);
return result;

tfjs-core/src/ops/image/threshold.ts, line 86 at r4 (raw file):

  } else if (method === 'otsu') {
    const histogram = Array(256).fill(0);

Maybe you can use tfjs.bincount for getting the histogram?


tfjs-core/src/ops/image/threshold.ts, line 95 at r4 (raw file):

    const threshold = otsu_alg(histogram, arrayedImage.length);

    arrayedImage.forEach((item, i) => {

What about inverted?


tfjs-core/src/ops/image/threshold.ts, line 96 at r4 (raw file):

    arrayedImage.forEach((item, i) => {
      arrayedImage[i] = (item < threshold) ? 0 : 255;

Can you use tfjs ops same as the basic case.

Code changed according to review suggestions.
Update threshold.ts
Code changed according to review suggestions.
changed return to "result as Tensor3D;" because of unexpecred error.
Update threshold.ts
Changed return to "result as Tensor3D;" because of unexpecred error.
Copy link
Collaborator

@lina128 lina128 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 @BadMachine)


tfjs-core/src/ops/ops.ts, line 303 at r7 (raw file):

// Second level exports.
export {image, linalg, losses, spectral, fused, signal, sparse};

Why is sparse removed?


tfjs-core/src/ops/image/threshold.ts, line 96 at r7 (raw file):

        grayscale = add(add($r, $g), $b);
    } else {
        grayscale = image;

What is this condition for? When color channel is 4, what happens? If the method doesn't support the case, it's better to throw an error early on.


tfjs-core/src/ops/image/threshold_test.ts, line 23 at r7 (raw file):

describeWithFlags('threshold', ALL_ENVS, () => {
	it('default method binary, no arguments passed but input image', async () => {

Why is there '>>' sign?

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Hi @BadMachine , once you addressed the three comments, I think the PR is good to go, great work!

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

@BadMachine
Copy link
Contributor Author

BadMachine commented Apr 23, 2021

@lina128 Thanks for your attention!

Why is sparse removed?

Have no idea, didn't do anything except importing threshold op.

Why is there '>>' sign?

Excuse me, what did you mean by '>>' sign?

Copy link
Collaborator

@lina128 lina128 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 @BadMachine)


tfjs-core/src/ops/ops.ts, line 300 at r8 (raw file):

import {sparseReshape} from './sparse/sparse_reshape';
const sparse = {sparseReshape};

This two lines are still removed. Please add back.

@lina128
Copy link
Collaborator

lina128 commented Apr 23, 2021

This is what I see in Reviewable UI, see the '<<' signs, please reformat:

Screen Shot 2021-04-23 at 2 40 16 PM

@BadMachine
Copy link
Contributor Author

@lina128
Gotcha!

Copy link
Collaborator

@lina128 lina128 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 @BadMachine)


tfjs-core/src/ops/image/threshold_test.ts, line 23 at r7 (raw file):

Previously, lina128 (Na Li) wrote…

Why is there '>>' sign?

I think you use 2 tabs vs. 2 spaces.

@BadMachine
Copy link
Contributor Author

@lina128

What is this condition for? When color channel is 4, what happens? If the method doesn't support the case, it's better to throw an error early on.

Channels check has been added.

    util.assert(
        $image.shape[2] === 3 || $image.shape[2]=== 1,
        () => 'Error in threshold: ' +
            'image color channel must be equal to 3 or 1' +
            `but got ${$image.shape[2]}.`);

@BadMachine
Copy link
Contributor Author

@lina128

Also added threshold_test.ts
Extended /ops/ops.ts
Imported './ops/image/threshold_test' to tests.ts

But for some reason building failed with ERROR: build step 10 "node:10" failed: step exited with non-zero status: 1

Copy link
Collaborator

@lina128 lina128 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 @BadMachine)


tfjs-core/src/ops/image/threshold.ts, line 130 at r10 (raw file):

    for (let index = 0; index < histogram.size-1; index++) {

        classFirst = histogram.slice(0, index + 1);

Remove chained op.


tfjs-core/src/ops/image/threshold.ts, line 132 at r10 (raw file):

        classFirst = histogram.slice(0, index + 1);

        classSecond = histogram.slice(index + 1);

Remove chained op.

@BadMachine
Copy link
Contributor Author

@lina128

Remove chained op.

Done.

Copy link
Collaborator

@lina128 lina128 left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @BadMachine! LGTM!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @BadMachine)

@lina128 lina128 merged commit 2a30c2f into tensorflow:master Apr 23, 2021
@BadMachine
Copy link
Contributor Author

@lina128

Yippee! )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants