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

Batch support for deterministic image ops #15726

Merged
merged 6 commits into from Feb 15, 2018

Conversation

JoshVarty
Copy link
Contributor

Based on #14854 and working on #8926

Previously I had implemented batch support for a number of image ops. However performance concerns were raised and the changes were reverted.

I have re-implemented the changes for:

  • flip_left_right
  • flip_up_down
  • transpose_image
  • rot90

I ran performance tests from #15348 with:

bazel run -c opt //tensorflow/python:image_ops_test -- --benchmarks=FlipImageBenchmark

Operation Before (μs) After (μs)
benchmarkFlipLeftRight_299_299_3_/cpu:0_1 274.49 264.26
benchmarkFlipLeftRight_299_299_3_/cpu:0__all 292.76 266.10
benchmarkFlipLeftRight_299_299_3___all 273.80 265.71
*benchmarkRandomFlipLeftRight_299_299_3_/cpu:0_1 242.58 241.89
*benchmarkRandomFlipLeftRight_299_299_3_/cpu:0__all 245.27 239.88
*benchmarkRandomFlipLeftRight_299_299_3___all 252.71 241.20

* There were no changes made to `RandomFlipLeftRight in this PR

Let me know if you would like me to add more performance tests for the other methods. I don't think there should be any performance impact, but I'm happy to add more if you'd like.

@JoshVarty
Copy link
Contributor Author

/cc @martinwicke @jhseu

I would still like to include support for the random flip ops as well, but I'll do that in another PR if this looks acceptable.

@drpngx drpngx added the awaiting review Pull request awaiting review label Dec 31, 2017
@rmlarsen
Copy link
Member

@martinwicke could you take a look, please?

@tensorflowbutler
Copy link
Member

Nagging Assignee: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.

@martinwicke
Copy link
Member

I'm sorry this took me so long. Could you fix the conflicts? I think it's just that proper name_scopes were added to all functions in this module, which is nice and should be easy to merge.

@martinwicke martinwicke added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Feb 8, 2018
if shape.ndims == 3 or shape.ndims is None:
return _rot90_3D(image, k, scope)
elif shape.ndims == 4:
return _rot90_4D(image, k, scope)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing two methods for _rot90_3D and _rot90_4D is unfortunate. I'd definitely welcome a better suggestion. The other alternative was to stagger if shape.ndims == 3 checks throughout the method which didn't really help readability either.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine.

@JoshVarty
Copy link
Contributor Author

@martinwicke I've merged the commits and I think I've done so properly. All the tests pass and there are still no performance regressions so it's probably ready for a code review.

I also introduced _AssertAtLeast3DImage() in the style of _Assert3DImage() which had been added since I originally submitted this PR.

@martinwicke martinwicke added awaiting testing (then merge) and removed stat:awaiting response Status - Awaiting response from author labels Feb 14, 2018
@martinwicke
Copy link
Member

Sanity complains about indentation:

FAIL: Found 5 non-whitelited pylint errors:
tensorflow/python/ops/image_ops_impl.py:422: [C0330(bad-continuation), ] Wrong continued indentation (add 2 spaces).

tensorflow/python/ops/image_ops_impl.py:428: [C0330(bad-continuation), ] Wrong continued indentation (add 3 spaces).

tensorflow/python/ops/image_ops_impl.py:437: [C0301(line-too-long), ] Line too long (82/80)

tensorflow/python/ops/image_ops_impl.py:458: [C0330(bad-continuation), ] Wrong continued indentation (add 3 spaces).

tensorflow/python/ops/image_ops_test.py:1125: [C0330(bad-continuation), ] Wrong continued indentation (add 22 spaces).

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@martinwicke martinwicke merged commit 953e670 into tensorflow:master Feb 15, 2018
@JoshVarty JoshVarty deleted the BatchSimpleImageOps branch February 16, 2018 00:37
@JoshVarty
Copy link
Contributor Author

Sorry for the delay fixing indentation. I forgot to run the sanity checks locally. :(

Thanks for taking care of it!

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

7 participants