Add four global data augmentations for 3D #1028

lengzq · 2022-11-17T22:16:12Z

What does this PR do?

Fixes # (issue)

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline,
Pull Request section?
Was this discussed/approved via a Github issue? Please add a link
to it if that's the case.
Did you write any new necessary tests?
If this adds a new model, can you run a few training steps on TPU in Colab to ensure that no XLA incompatible OP are used?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

2. Add wrap_angle_rad helper function.

2. Change rads -> radians 3. Good point. We could add X or Y rotation as future work. For this PR, let's check in Z rotation first.

lengzq · 2022-11-17T22:16:40Z

@tanzhenyu

tanzhenyu

Thanks for the PR, looking great!

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

tanzhenyu · 2022-11-18T17:29:36Z

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

+    During inference time, the output will be identical to input. Call the layer with `training=True` to drop the input points.
+
+    Input shape:
+      point_clouds: 3D (multi frames) float32 Tensor with shape


I wonder if we need to support 4D tensor [batch_size, num_frames, num_points, point_feat]?
Asking user to do tf.map_fn on their own might be too much cognitive load, WDYT?

Yes, it is supported. Please see the test case, where I tested augmentation with a batch dimension.

tanzhenyu · 2022-11-18T17:31:37Z

keras_cv/layers/preprocessing3d/global_random_flipping_y.py

+BOUNDING_BOXES = base_augmentation_layer_3d.BOUNDING_BOXES
+
+
+class GlobalRandomFlippingY(base_augmentation_layer_3d.BaseAugmentationLayer3D):


just a discussion here: since we use "horizontal" and "vertical" in keras layers and Tensorflow in general, would it be better to be consistent with that?
https://keras.io/api/layers/preprocessing_layers/image_preprocessing/random_flip/
https://www.tensorflow.org/api_docs/python/tf/image/random_flip_left_right

I'm not sure if that image concept applies to point concept though

The point coordinators are defined based on (X, Y, Z) instead of (row, col). I think it is better to use 'Y' instead of 'horizontal/vertical'.

sounds good

I think for consistency with the 2D equivalent we should call this GlobalRandomFlipY instead of GlobalRandomFlippingY

tanzhenyu · 2022-11-18T17:32:51Z

keras_cv/layers/preprocessing3d/global_random_flipping_y.py

+      point_clouds: 3D (multi frames) float32 Tensor with shape
+        [num of frames, num of points, num of point features].
+        The first 5 features are [x, y, z, class, range].
+      bounding_boxes: 3D (multi frames) float32 Tensor with shape


@ianstenbit FYI, this would be a future improvement from our side to introduce 3d box format, given we already have 2d box format

+1 let's discuss this when the input pipeline is ready.

Agreed -- we should support box format here, even if we only have 1 format. Once we have a training script skeleton set up I can set up the box format infra.

tanzhenyu · 2022-11-18T17:33:45Z

keras_cv/layers/preprocessing3d/global_random_scaling.py

+      max_scaling_factor: A float scaler or Tensor sets the maximum scaling factor.
+    """
+
+    def __init__(self, min_scaling_factor, max_scaling_factor, **kwargs):


Is this a scalar, or a vector of size 3?

scalar. We scale (x, y, z) using a single scalar.

will it need to support scaling differently on different axes?

keras_cv/layers/preprocessing3d/global_random_translation.py

tanzhenyu · 2022-11-21T18:49:40Z

keras_cv/layers/preprocessing3d/global_random_scaling.py

+
+    def __init__(
+        self,
+        min_scaling_factor_x,


nit: maybe we can make this consistent with randomzoom? https://www.tensorflow.org/api_docs/python/tf/keras/layers/RandomZoom

So it's scaling_factor_x -- a tuple of size 2, if it means [min_x, max_x], or a single scalar, if it means the factor is fixed

ianstenbit

Thanks for the PR! These are great -- looking forward to getting them merged :)

ianstenbit · 2022-11-21T18:50:05Z

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

+      A tuple of two Tensors (point_clouds, bounding_boxes) with the same shape as input Tensors.
+
+    Arguments:
+      keep_probability: A float scaler or Tensor sets the probability threshold for keeping the points.


How would you feel about changing this argument to drop_rate (and inverting the behavior such that high drop rate ~ low keep probability)
This seems a bit more consistent with the name of the layer as well as related conventions like dropout

ianstenbit · 2022-11-21T18:51:37Z

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

+        The first 7 features are [x, y, z, dx, dy, dz, phi].
+
+    Output shape:
+      A tuple of two Tensors (point_clouds, bounding_boxes) with the same shape as input Tensors.


I think the output is really a dictionary, right?

(Same for the other 3 layers)

I thought the output shape is for augment_point_clouds_bounding_boxes function. Let me update all of them.

ianstenbit · 2022-11-21T18:52:42Z

keras_cv/layers/preprocessing3d/global_random_flipping_y.py

+BOUNDING_BOXES = base_augmentation_layer_3d.BOUNDING_BOXES
+
+
+class GlobalRandomFlippingY(base_augmentation_layer_3d.BaseAugmentationLayer3D):


I think for consistency with the 2D equivalent we should call this GlobalRandomFlipY instead of GlobalRandomFlippingY

ianstenbit · 2022-11-21T18:53:56Z

keras_cv/layers/preprocessing3d/global_random_dropping_points_test.py

+        outputs = add_layer(inputs)
+        self.assertNotAllClose(inputs, outputs)
+
+    def test_not_augment_batch_point_clouds_and_bounding_boxes(self):


Optional: consider dropping this test case and the one below it.

I think this test and the one below it don't add any coverage, since we already have test cases for dropping all and dropping none of the points, as well as a single test for batched augmentation.

ianstenbit · 2022-11-21T18:54:34Z

keras_cv/layers/preprocessing3d/global_random_flipping_y.py

+      point_clouds: 3D (multi frames) float32 Tensor with shape
+        [num of frames, num of points, num of point features].
+        The first 5 features are [x, y, z, class, range].
+      bounding_boxes: 3D (multi frames) float32 Tensor with shape


Agreed -- we should support box format here, even if we only have 1 format. Once we have a training script skeleton set up I can set up the box format infra.

ianstenbit · 2022-11-21T18:57:32Z

keras_cv/layers/preprocessing3d/global_random_scaling.py

+
+    def __init__(
+        self,
+        min_scaling_factor_x,


I think we should make these tuples like:
x_factor (which is a tuple of (min, max))

As a user of this API, I'd also expect to be able to pass y_factor=None (probably should be a default parameter value) to indicate that I want no scaling on the y axis

ianstenbit · 2022-11-21T18:59:12Z

keras_cv/layers/preprocessing3d/global_random_scaling_test.py

+
+
+class GlobalScalingTest(tf.test.TestCase):
+    def test_augment_point_clouds_and_bounding_boxes(self):


(Same comment as the random flipping -- I think we should have a test case here with a positive assertion about the numerics of the scaled output)

e.g.
input_points = [some known set of points]
output_points = [the expected values of the scaled points]

Added test_2x_scaling_point_clouds_and_bounding_boxes test case.

awesome -- I still think it would be good to do this for flipping and rotation layers as well

ianstenbit · 2022-11-21T18:59:43Z

keras_cv/layers/preprocessing3d/global_random_translation.py

+    Arguments:
+      x_translation_stddev: A float scaler or Tensor sets the translation noise standard deviation along the X axis.
+      y_translation_stddev: A float scaler or Tensor sets the translation noise standard deviation along the Y axis.
+      z_translation_stddev: A float scaler or Tensor sets the translation noise standard deviation along the Z axis.


nit (throughout the PR): s/scaler/scalar

ianstenbit · 2022-11-21T19:00:48Z

keras_cv/layers/preprocessing3d/global_random_translation.py

+    """
+
+    def __init__(
+        self, x_translation_stddev, y_translation_stddev, z_translation_stddev, **kwargs


Here I'd also expect to be able to use a default y_translation_stddev=None to indicate that no translation on the y axis should occur.

I added

x_stddev = x_stddev if x_stddev else 0.0 y_stddev = y_stddev if y_stddev else 0.0 z_stddev = z_stddev if z_stddev else 0.0

ianstenbit · 2022-11-21T19:01:30Z

keras_cv/layers/preprocessing3d/global_random_translation.py

+    """
+
+    def __init__(
+        self, x_translation_stddev, y_translation_stddev, z_translation_stddev, **kwargs


What do you think about calling these x_magnitude or something like that (just trying to come up with something a bit more brief)

I can change x_translation_stddev to x_stddev. What do you think?

yeah that sgtm

tanzhenyu · 2022-11-21T20:43:18Z

/gcbrun

tanzhenyu · 2022-11-21T20:48:30Z

/gcbrun

ianstenbit

LGTM, just a few comments.

Thank you!

ianstenbit · 2022-11-21T21:16:48Z

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

+        super().__init__(**kwargs)
+        keep_probability = 1 - drop_rate
+        if keep_probability < 0:
+            raise ValueError("keep_probability must be >=0.")


nit: since drop_rate is what's in the API, maybe this should say "drop rate must be <= 1"

Good point. Done.

ianstenbit · 2022-11-21T21:17:47Z

keras_cv/layers/preprocessing3d/global_random_flip_y.py

+    def augment_point_clouds_bounding_boxes(
+        self, point_clouds, bounding_boxes, transformation, **kwargs
+    ):
+        del transformation


just curious -- is this necessary?

ianstenbit · 2022-11-21T21:18:54Z

keras_cv/layers/preprocessing3d/global_random_scaling.py

+
+    def __init__(
+        self,
+        scaling_factor_x,


(here and in the other KPLs where you've added defaults) -- let's make this default to None in the constructor like

scaling_factor_x=None

ianstenbit · 2022-11-21T21:19:35Z

keras_cv/layers/preprocessing3d/global_random_translation.py

+    """
+
+    def __init__(
+        self, x_translation_stddev, y_translation_stddev, z_translation_stddev, **kwargs


yeah that sgtm

ianstenbit · 2022-11-21T21:20:33Z

keras_cv/layers/preprocessing3d/global_random_scaling_test.py

+
+
+class GlobalScalingTest(tf.test.TestCase):
+    def test_augment_point_clouds_and_bounding_boxes(self):


awesome -- I still think it would be good to do this for flipping and rotation layers as well

bhack · 2022-11-21T22:44:26Z

Is this PR growing too much? Why we have not contributed each augmentation in a separate PR?

ianstenbit · 2022-11-21T23:24:05Z

keras_cv/layers/preprocessing3d/global_random_dropping_points.py

        super().__init__(**kwargs)
+        drop_rate = drop_rate if drop_rate else 0.0
+
+        if drop_rate <= 1:


This should be if drop_rate > 1

(Hopefully we'll have a test failure indicating this?)

Sorry for the typo.

ianstenbit · 2022-11-21T23:24:48Z

keras_cv/layers/preprocessing3d/global_random_scaling.py

-      scaling_factor_x: A tuple of float scalar sets the minimum and maximum scaling factors for the X axis.
-      scaling_factor_y: A tuple of float scalar sets the minimum and maximum scaling factors for the Y axis.
-      scaling_factor_z: A tuple of float scalar sets the minimum and maximum scaling factors for the Z axis.
+      scaling_factor_x: A tuple of float scalar or a float scaler sets the minimum and maximum scaling factors for the X axis.


nit: s/scaler/scalar

ianstenbit · 2022-11-21T23:49:38Z

/gcbrun

tanzhenyu · 2022-11-22T00:09:41Z

Is this PR growing too much? Why we have not contributed each augmentation in a separate PR?

yeah it'd be nice to have separate PRs

tanzhenyu · 2022-11-22T00:25:58Z

It's again timing out, so merging it manually

* Add base augmentation layer for 3D preception. * Fix format. * Add copyright. * Minor change. * revert the minor change in the test file. * 1. Add global_z_rotation data augmentation. 2. Add wrap_angle_rad helper function. * Auto format. * 1. Standardize POINT_CLOUDS and BOUNDING_BOXES 2. Change rads -> radians 3. Good point. We could add X or Y rotation as future work. For this PR, let's check in Z rotation first. * Format. * Delete base_augmentation_layer_3d.py * Delete base_augmentation_layer_3d_test.py * Standardize POINT_CLOUDS and BOUNDING_BOXES names. * Change GlobalZRotation to GlobalRandomZRotation * Support rotation along X, Y and Z axes. * format. * Change file name from global_rotation to global_random_rotation. * Add four more global data augmentations for 3d. * format. * Remove unused import. * Fix a typo in GlobalRandomFlippingY. * Support scaling x, y, and z. * Format. * update random scaling. * Modified based on comments. * follow up. * Fix a typo in random_scaling_test.py * Update. * Fix two typos. Co-authored-by: Leng Zhaoqi <[email protected]>

Leng Zhaoqi and others added 20 commits November 4, 2022 15:56

Add base augmentation layer for 3D preception.

3221849

Fix format.

4e032c1

Add copyright.

30af449

Minor change.

f73e2ff

revert the minor change in the test file.

bd26fef

1. Add global_z_rotation data augmentation.

3f534d5

2. Add wrap_angle_rad helper function.

Auto format.

693fab0

1. Standardize POINT_CLOUDS and BOUNDING_BOXES

8152d70

2. Change rads -> radians 3. Good point. We could add X or Y rotation as future work. For this PR, let's check in Z rotation first.

Format.

55a2ffb

Delete base_augmentation_layer_3d.py

6f1379c

Delete base_augmentation_layer_3d_test.py

e9a48e0

Merge branch 'keras-team:master' into master

b99e351

Standardize POINT_CLOUDS and BOUNDING_BOXES names.

86482f1

Merge branch 'keras-team:master' into master

1a06fb1

Change GlobalZRotation to GlobalRandomZRotation

b556f84

Support rotation along X, Y and Z axes.

0cd396f

format.

19123b9

Change file name from global_rotation to global_random_rotation.

de19450

Merge branch 'keras-team:master' into master

da6cc6d

Add four more global data augmentations for 3d.

eef63ac

tanzhenyu requested review from tanzhenyu and ianstenbit November 17, 2022 22:17

Leng Zhaoqi added 3 commits November 17, 2022 14:23

format.

2bb0c55

Remove unused import.

54439f7

Fix a typo in GlobalRandomFlippingY.

c577abc

tanzhenyu suggested changes Nov 18, 2022

View reviewed changes

Leng Zhaoqi added 2 commits November 21, 2022 09:57

Support scaling x, y, and z.

5ff7596

Format.

1223912

tanzhenyu reviewed Nov 21, 2022

View reviewed changes

ianstenbit suggested changes Nov 21, 2022

View reviewed changes

Leng Zhaoqi added 3 commits November 21, 2022 11:02

update random scaling.

eb3bc17

Modified based on comments.

39755e0

follow up.

4d4f0a6

tanzhenyu approved these changes Nov 21, 2022

View reviewed changes

Fix a typo in random_scaling_test.py

7a90015

ianstenbit approved these changes Nov 21, 2022

View reviewed changes

Update.

4d8cdef

ianstenbit approved these changes Nov 21, 2022

View reviewed changes

Fix two typos.

d343dd7

tanzhenyu merged commit 87f6026 into keras-team:master Nov 22, 2022

		BOUNDING_BOXES = base_augmentation_layer_3d.BOUNDING_BOXES


		class GlobalRandomFlippingY(base_augmentation_layer_3d.BaseAugmentationLayer3D):



		class GlobalScalingTest(tf.test.TestCase):
		def test_augment_point_clouds_and_bounding_boxes(self):

Add four global data augmentations for 3D #1028

Add four global data augmentations for 3D #1028

Conversation

lengzq commented Nov 17, 2022 • edited Loading

What does this PR do?

Before submitting

Who can review?

lengzq commented Nov 17, 2022

tanzhenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lengzq Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianstenbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanzhenyu commented Nov 21, 2022

tanzhenyu commented Nov 21, 2022

ianstenbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhack commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianstenbit commented Nov 21, 2022

tanzhenyu commented Nov 22, 2022

tanzhenyu commented Nov 22, 2022

lengzq commented Nov 17, 2022 •

edited

Loading

lengzq Nov 18, 2022 •

edited

Loading