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.

Sign up for GitHub

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

Jump to bottom

Update the GridMask layer to use BaseImageAugmentationLayer #237

Closed
wants to merge 1 commit into from
Closed

Update the GridMask layer to use BaseImageAugmentationLayer #237

wants to merge 1 commit into from

Conversation

qlzh727
Copy link
Member

@qlzh727 qlzh727 commented Mar 29, 2022

Fix #190

@qlzh727 qlzh727 requested a review from LukeWood March 29, 2022 22:21
LukeWood
LukeWood reviewed Mar 30, 2022
View reviewed changes
keras_cv/layers/preprocessing/grid_mask.py
),
gridblock - 1,
grid_block - 1,
),
tf.int32,
)

for _ in range(2):
start_x = tf.random.uniform(
Copy link
Contributor

Choose a reason for hiding this comment

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

should this use _random_generator too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also - can we move compute mask to get_random_transformation or were there some sort of vectorization issues requiring it to be in the main func?

@chjort
Copy link
Contributor

chjort commented Mar 30, 2022 •
edited
Loading

Moving GridMask to BaseImageAugmentationLayer is also being done in #159. And the implementation in also makes use of fill_utils.py. #159 originally proposed a fully vectorized implementation of GridMask, but has now been migrated to use BaseImageAugmentationLayer as per request.

Auto vectorization however is not yet functional using BaseImageAugmentationLayer, and I am uncertain if it is possible for GridMask since it uses RandomRotation layer. It is being worked on though.

Let me know if #159 is still relevant or if I should close it in light of this PR.

@chjort chjort mentioned this pull request Mar 30, 2022
Merged
@qlzh727
Copy link
Member Author

qlzh727 commented Mar 30, 2022

Moving GridMask to BaseImageAugmentationLayer is also being done in #159. And the implementation in also makes use of fill_utils.py. #159 originally proposed a fully vectorized implementation of GridMask, but has now been migrated to use BaseImageAugmentationLayer as per request.

Auto vectorization however is not yet functional using BaseImageAugmentationLayer, and I am uncertain if it is possible for GridMask since it uses RandomRotation layer. It is being worked on though.

Let me know if #159 is still relevant or if I should close it in light of this PR.

I see. I think your PR is nice since it further cleanup the cl for mask generation. Let me review your PR instead.

@qlzh727 qlzh727 closed this Mar 30, 2022
@qlzh727 qlzh727 deleted the grid_mask branch September 20, 2022 17:49
freedomtan pushed a commit to freedomtan/keras-cv that referenced this pull request Jul 20, 2023
@mattdangerw
Fix arange to have the same defaults as numpy (keras-team#237)
764b285
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukeWood LukeWood LukeWood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate GridMask to use BaseImageAugmentationLayer
3 participants
@qlzh727 @chjort @LukeWood