Skip to content

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Dec 6, 2019

BUG

Fix bug in CropAndResize, and add coverage in core. Also add AutoML models to benchmark.


This change is Reviewable

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1, 1 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan and @nsthorat)

Copy link
Contributor

@annxingyuan annxingyuan 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: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan and @dsmilkov)


tfjs-backend-wasm/src/cc/kernels/CropAndResize.cc, line 55 at r2 (raw file):

    for (int c = 0; c < num_channels; ++c) {
      const int in_ind = c + closest_x * images_strides[2] +
                         closest_y * images_strides[1] + box_ind;

Now that box_ind refers to the offset, should we change the logic here? Same with the interpolate_bilinear shared utility.

Copy link
Contributor Author

@dsmilkov dsmilkov 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: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan)


tfjs-backend-wasm/src/cc/kernels/CropAndResize.cc, line 55 at r2 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

Now that box_ind refers to the offset, should we change the logic here? Same with the interpolate_bilinear shared utility.

nice catch. reverted to box_offset

@dsmilkov dsmilkov merged commit 10902ae into master Dec 6, 2019
@dsmilkov dsmilkov deleted the bench-automl branch December 6, 2019 20:11
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.

4 participants