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

feat(layers): Migrate RandomChoice and RandomApply layers from keras-cv to keras #20752

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

harshaljanjani
Copy link
Contributor

  • Added RandomChoice and RandomApply layers to keras.layers.preprocessing.image_preprocessing
  • Implemented necessary transform methods (transform_images, transform_labels, transform_bounding_boxes, transform_segmentation_masks) to comply with BaseImagePreprocessingLayer
  • Updated tests to ensure compatibility with keras and fix failing test cases
  • Added support for batchwise processing, auto-vectorization, and random seed control
  • Addresses add RandomApply and RandomChoice #20727

- Added RandomChoice and RandomApply layers to keras.layers.preprocessing.image_preprocessing.
- Implemented necessary transform methods (transform_images, transform_labels, transform_bounding_boxes, transform_segmentation_masks) to comply with BaseImagePreprocessingLayer.
- Updated tests to ensure compatibility with keras and fix failing test cases.
- Added support for batchwise processing, auto-vectorization, and random seed control.
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 79.31034% with 24 lines in your changes missing coverage. Please review.

Project coverage is 82.01%. Comparing base (e010829) to head (595104d).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
.../preprocessing/image_preprocessing/random_apply.py 77.94% 10 Missing and 5 partials ⚠️
...preprocessing/image_preprocessing/random_choice.py 80.43% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20752      +/-   ##
==========================================
+ Coverage   81.94%   82.01%   +0.06%     
==========================================
  Files         553      557       +4     
  Lines       51536    51972     +436     
  Branches     7978     8034      +56     
==========================================
+ Hits        42231    42623     +392     
- Misses       7363     7393      +30     
- Partials     1942     1956      +14     
Flag Coverage Δ
keras 81.83% <79.31%> (+0.06%) ⬆️
keras-jax 64.15% <79.31%> (+0.16%) ⬆️
keras-numpy 58.96% <79.31%> (+0.11%) ⬆️
keras-openvino 29.90% <25.86%> (+0.09%) ⬆️
keras-tensorflow 64.78% <79.31%> (+0.10%) ⬆️
keras-torch 64.22% <79.31%> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…andomApply and RandomChoice layers

- Updated layers to handle JAX backend-specific behavior and avoid tracer leaks.
- Updated tests to use backend-agnostic Keras operations.
- Skipped JAX-specific tests with TODO to investigate further.
- Removed Tensorflow-specific dependencies for better backend compatibility.
- Add handling for layers with get_random_transformation method in both
  RandomApply and RandomChoice preprocessing layers
- Fix seed handling in RandomChoice to ensure consistent behavior across backends
- Remove JAX-specific test skipmarks after addressing tracer errors
@harshaljanjani harshaljanjani requested a review from innat January 14, 2025 13:34
- Simplified docstrings.
- Removed copyright headers from files.
- Refactored RandomApply and RandomChoice.
- Streamlined transformation logic and improved overall code organization.
- Eliminated redundant backend-specific code paths.
@harshaljanjani
Copy link
Contributor Author

harshaljanjani commented Jan 18, 2025

@fchollet, would love your thoughts on these changes whenever you get a chance, thanks! Also please do look into the comment I've left unresolved regarding whether it should inherit from keras.layers.Layer instead.

Copy link
Collaborator

@fchollet fchollet 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!

@innat do you want to review?

@innat
Copy link

innat commented Jan 18, 2025

Sure, I'll review it. Thanks for tagging me.

- removed unused auto_vectorize parameter from both layers
- shortened docstring headers to fit on one line
- added backticks around boolean keywords (True, False) in docstrings
- fixed incorrect transform methods (bounding_boxes, labels, segmentation_masks)
  to return inputs unchanged
- removed unnecessary transformation methods from RandomChoice for consistency
  with original implementation
@harshaljanjani
Copy link
Contributor Author

harshaljanjani commented Jan 18, 2025

Thanks for the reviews, @fchollet and @innat. I've tried to incorporate the suggested changes, will check the results of the test cases as soon as I have the opportunity!
Edit: The test cases have passed. Please take a look when you have the chance. Thanks!

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

Successfully merging this pull request may close these issues.

5 participants