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

Support convert from UpperLeft, Center in BoundingBoxUtils #15

Merged
merged 3 commits into from
Oct 12, 2020
Merged

Support convert from UpperLeft, Center in BoundingBoxUtils #15

merged 3 commits into from
Oct 12, 2020

Conversation

am15h
Copy link
Contributor

@am15h am15h commented Jul 1, 2020

No description provided.

Copy link
Contributor

@xunkai55 xunkai55 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the late reply. Please take a look.

return new RectF(
left, top, right, bottom);
} else {
throw new IllegalArgumentException("BoundingBox.Type Center is not supported with CoordinateType CoordinateType.RATIO");
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, it looks like able to be supported - perhaps leave a TODO here if you don't want to put all in one PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will just add it here. Please confirm if with CoordinateType.RATIO, centerX = values[0] * width, centerY = values[1] * height . What will values[2], values[3] depict? (as we will be using height and width from parameters)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xunkai55 Thanks for reviewing, I have made other changes, please check.

Copy link
Contributor

Choose a reason for hiding this comment

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

values[2] and [3] represent the relative width and height of the bounding box. So the height and width of the box should be values[2] * width, or imageWidth as I commented in another thread.

left, top, left + w, top + h);
} else {
return new RectF(
values[0] * width, values[1] * height, width * values[2], height * values[3]);
Copy link
Contributor

@xunkai55 xunkai55 Aug 5, 2020

Choose a reason for hiding this comment

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

For readability, I'd suggest to move "left", "top", "w", "h" out of the conditional scope, and not use value[i] directly. Argument of the function height and width maybe should be renamed to imageHeight and imageWidth to avoid confusion.

And particularly in this line, let's keep expressions in a same order, e.g. foo * imageHeight or foo * imageWidth.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct expressions seem to be

values[0] * imageWidth
values[1] * imageHeight
(values[0] + values[2]) * imageWidth
(values[1] + values[3]) * imageHeight

float w = values[2];
float h = values[3];

float left = centerX - w/2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add spaces around operator /.

float w = values[2];
float h = values[3];

return new RectF(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: New line here is not needed.

float right = centerX + w/2;
float bottom = centerX + h/2;

return new RectF(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: New line is not needed.

Copy link
Contributor

@xunkai55 xunkai55 left a comment

Choose a reason for hiding this comment

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

Thanks for the effort! Truly sorry to be late.

@am15h
Copy link
Contributor Author

am15h commented Aug 12, 2020

Thanks a lot @xunkai55 for reviewing, I have made changes according to your suggestions. Very sorry for the delay

@xunkai55 xunkai55 added the ready to pull PR is reviewed and ready to be integrated label Sep 6, 2020
@copybara-service copybara-service bot merged commit b1504ee into tensorflow:master Oct 12, 2020
@xunkai55
Copy link
Contributor

Thank you @am15h for the contribution. The PR is merged now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR is reviewed and ready to be integrated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants