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 fill rectangle #65

Merged
merged 9 commits into from
Jan 30, 2022
Merged

Conversation

chjort
Copy link
Contributor

@chjort chjort commented Jan 30, 2022

Vectorized/batched implementation of fill_rectangle for significant performance improvements.

This implementation has been benchmarked against the non-vectorized implementation of fill_rectangle that uses tf.map_fn.

The following table summarizes the time (in milliseconds) it takes to fill a batch of rectangles into a batch of 32x32 images. These are the numbers I get on my own machine using GPU.

Batch sizes Batch fill time [milliseconds] Map fill time [milliseconds]
1 0.5903 0.8888
2 0.6151 1.1497
4 0.5357 1.5249
8 0.5238 2.8148
16 0.5359 5.3349
32 0.5364 9.8739
64 0.5424 19.0392
128 0.5553 46.8254
256 0.5877 89.102
512 0.6801 136.8117
1024 0.6406 279.6941
2048 0.6969 588.2947
4096 0.7975 1190.1174

And plot:
fill_benchmark

@chjort
Copy link
Contributor Author

chjort commented Jan 30, 2022

I don't know why black is failing on these files:

  • keras_cv/metrics/coco/base_test.py
  • keras_cv/metrics/coco/base.py
  • keras_cv/metrics/coco/numerical_tests/recall_correctness_test.py
  • keras_cv/metrics/coco/recall_test.py

I have not touched those, and when i run black --check --line-length 85 . locally I get no errors. Is something wrong with the Actions workflow?

@LukeWood
Copy link
Contributor

@chjort I added a commit to fix the code. I'll also be changing formatting rules later tonight anyways. The 85 character line length we don't really want anyways.

@LukeWood
Copy link
Contributor

to solve the issue upgrade black)

@LukeWood
Copy link
Contributor

@chjort this is a great, high quality contribution. Thanks for this. Do you have any interest in contributing some test cases? In particular, I'm interested in adding:

  • edge case where the center_x+width > image_width
  • edge case where the center_x-width < 0
  • and the same for height

Thanks @chjort, thanks again

@LukeWood
Copy link
Contributor

Note: I locally ran the demo scripts and they still work.

@LukeWood LukeWood self-requested a review January 30, 2022 03:34
@LukeWood
Copy link
Contributor

Actually, this is untested as of now regardless. That's something I will document, but I won't block the PR on it.

@LukeWood LukeWood merged commit ade808c into keras-team:master Jan 30, 2022
@LukeWood
Copy link
Contributor

Thanks @chjort - you rock!

keras_cv/utils/fill_utils.py Show resolved Hide resolved
keras_cv/utils/fill_utils.py Show resolved Hide resolved
@chjort
Copy link
Contributor Author

chjort commented Jan 30, 2022

@LukeWood I'm glad you find it useful. You're welcome!

I'd be happy to contribute some tests, including the cases you mentioned.

In this new contribution I will also resolve the things you pointed out @bhack

@chjort chjort deleted the batch_fill_rectangle branch February 3, 2022 13:54
@chjort chjort restored the batch_fill_rectangle branch February 3, 2022 13:55
@chjort chjort deleted the batch_fill_rectangle branch February 3, 2022 20:38
ianstenbit pushed a commit to ianstenbit/keras-cv that referenced this pull request Aug 6, 2022
* temporary push benchmark file

* temporary push benchmark file

* refactor fill_utils.py

* refactor

* refactor

* batch fill_rectangle

* small refactor

* docstring fix

* Fix issue after rebase

Co-authored-by: Luke Wood <lukewood@google.com>
adhadse pushed a commit to adhadse/keras-cv that referenced this pull request Sep 17, 2022
* temporary push benchmark file

* temporary push benchmark file

* refactor fill_utils.py

* refactor

* refactor

* batch fill_rectangle

* small refactor

* docstring fix

* Fix issue after rebase

Co-authored-by: Luke Wood <lukewood@google.com>
freedomtan pushed a commit to freedomtan/keras-cv that referenced this pull request Jul 20, 2023
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.

4 participants