[Clustering] Fixing the bug in clustering for Conv1D/Conv3D #609

wwwind · 2021-01-05T11:22:49Z

There is a bug in clustering for Conv1D/Conv3D. We used the layout for Conv2D and this is wrong, because tensors for Conv1D/ Conv3D have different ranks. Tests for both cases are added.

Change-Id: Ic1ee16760d9643b8142e5644d6cb68a6890bc621

akarmi

Thanks. Looks like we need a similar test and fix for Conv1D.

Change-Id: Ic20345f29b6548e7eabf34fe5488654284987a83

Change-Id: Icd0acb2c5a671d4cafc3222fcc4d8098716b3f73

Change-Id: If55e372e7f13fc032f4b85d7e449ed0ecaf6a873

Change-Id: Ib4331480e2d29a24973a15fb5d2cbcf05e051b91

akarmi · 2021-04-13T15:39:11Z

@psunn, please review

psunn · 2021-04-13T15:45:13Z

LGTM, thanks!

daverim · 2021-04-19T09:44:57Z

tensorflow_model_optimization/python/core/clustering/keras/cluster_test.py

@@ -199,6 +202,54 @@ def testDepthwiseConv2DLayerNonClusterable(self):
                                   wrapped_layer)
    self.assertEqual([], wrapped_layer.layer.get_clusterable_weights())

+  @keras_parameterized.run_all_keras_modes
+  def testConv1DLayer(self):
+     """


Nit: """Verifies that we can cluster a Conv1D layer.""" Should be a one-liner

daverim · 2021-04-19T09:45:16Z

tensorflow_model_optimization/python/core/clustering/keras/cluster_test.py

+     """
+     Verifies that we can cluster a Conv3D layer.
+     """
+     input_shape =(4, 28, 28, 28, 1)


Same as above:

daverim · 2021-04-19T09:46:57Z

tensorflow_model_optimization/python/core/clustering/keras/clustering_registry.py

+            [tf.reshape(self.cluster_centroids, [1 for _ in range(wt_dim-2)] + [clst_num])] *
+            weight.shape[-2], axis=wt_dim-2),
+                 [i for i in weight.shape[:-2]] + [1, 1])] * weight.shape[-1],
+        axis=wt_dim-1)


Seems a little clunky with all the list comprehensions, is it possible to make this a bit more readable?

1. Nits: Docstring corrections 2. Simplify clunky code in get_pulling_indices Change-Id: I3c6818099fab1c9b844620cd1dfad6e97efeb0fd

wwwind · 2021-04-19T15:12:19Z

Hi @daverim Thanks for the review! Your comments are addressed. Please take a look.

Change-Id: I367607cb123404f72da7b4a592a70d154d14ed13

daverim · 2021-05-10T07:34:10Z

Hey sorry, is this PR still required? Can you rebase?

google-cla · 2021-05-10T14:59:19Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2021-05-10T14:59:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

Change-Id: Ied79f7dfe484770d41dc9b08ae3b5acdbd970e54

wwwind · 2021-05-10T15:31:38Z

@googlebot I signed it!

googlebot · 2021-05-10T15:31:54Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-10T15:32:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

psunn · 2021-05-10T15:39:13Z

@googlebot I consent.

wwwind · 2021-05-10T15:43:10Z

@googlebot I consent.

googlebot · 2021-05-10T15:44:30Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

google-cla · 2021-05-10T15:44:34Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Fixed clustering for Conv3D.

19b5322

Change-Id: Ic1ee16760d9643b8142e5644d6cb68a6890bc621

googlebot added the cla: yes PR contributor has signed CLA label Jan 5, 2021

wwwind requested a review from akarmi January 5, 2021 11:22

github-actions bot added the technique:clustering Regarding tfmot.clustering.keras APIs and docs label Jan 5, 2021

akarmi suggested changes Feb 5, 2021

View reviewed changes

wwwind and others added 2 commits April 6, 2021 10:46

Merge branch 'upstream/master' into conv3d_bug_clustering

647d643

Change-Id: Ic20345f29b6548e7eabf34fe5488654284987a83

Added fix bug for clustering with Conv1D

6917424

Change-Id: Icd0acb2c5a671d4cafc3222fcc4d8098716b3f73

wwwind changed the title ~~[Clustering] Fixing the bug in clustering for Conv3D~~ [Clustering] Fixing the bug in clustering for Conv1D/Conv3D Apr 6, 2021

wwwind requested a review from akarmi April 6, 2021 10:31

wwwind added 3 commits April 13, 2021 13:51

Merge branch 'master' into conv3d_bug_clustering

8ff951c

Resolved small issues after updating with master.

0779443

Change-Id: If55e372e7f13fc032f4b85d7e449ed0ecaf6a873

Small fix after updating with master.

42f4558

Change-Id: Ib4331480e2d29a24973a15fb5d2cbcf05e051b91

akarmi approved these changes Apr 13, 2021

View reviewed changes

akarmi assigned akarmi and unassigned akarmi Apr 13, 2021

wwwind added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Apr 13, 2021

akarmi removed the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Apr 13, 2021

wwwind added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Apr 13, 2021

daverim reviewed Apr 19, 2021

View reviewed changes

Addressing comments:

41be265

1. Nits: Docstring corrections 2. Simplify clunky code in get_pulling_indices Change-Id: I3c6818099fab1c9b844620cd1dfad6e97efeb0fd

akarmi removed the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Apr 21, 2021

Removed clustering registry completely.

d2a916d

Change-Id: I367607cb123404f72da7b4a592a70d154d14ed13

github-actions bot added the api-review API review needed label Apr 23, 2021

Merge branch 'master' into conv3d_bug_clustering

3cc9cf7

Merge branch 'master' into conv3d_bug_clustering

fa56571

google-cla bot added cla: no PR contributor has not signed CLA and removed cla: yes PR contributor has signed CLA labels May 10, 2021

Small correction after merging with master.

f0f5342

Change-Id: Ied79f7dfe484770d41dc9b08ae3b5acdbd970e54

wwwind requested a review from akarmi May 10, 2021 15:15

googlebot added cla: yes PR contributor has signed CLA and removed cla: no PR contributor has not signed CLA labels May 10, 2021

akarmi approved these changes May 10, 2021

View reviewed changes

daverim approved these changes May 11, 2021

View reviewed changes

daverim added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label May 11, 2021

Merge branch 'master' into conv3d_bug_clustering

6b284bb

copybara-service bot merged commit 3e7f162 into tensorflow:master Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clustering] Fixing the bug in clustering for Conv1D/Conv3D #609

[Clustering] Fixing the bug in clustering for Conv1D/Conv3D #609

wwwind commented Jan 5, 2021 •

edited

akarmi left a comment

akarmi commented Apr 13, 2021

psunn commented Apr 13, 2021

daverim Apr 19, 2021

daverim Apr 19, 2021

daverim Apr 19, 2021

wwwind commented Apr 19, 2021

daverim commented May 10, 2021

google-cla bot commented May 10, 2021

googlebot commented May 10, 2021

wwwind commented May 10, 2021

googlebot commented May 10, 2021

google-cla bot commented May 10, 2021

psunn commented May 10, 2021 •

edited

wwwind commented May 10, 2021

googlebot commented May 10, 2021

google-cla bot commented May 10, 2021

[Clustering] Fixing the bug in clustering for Conv1D/Conv3D #609

[Clustering] Fixing the bug in clustering for Conv1D/Conv3D #609

Conversation

wwwind commented Jan 5, 2021 • edited

akarmi left a comment

Choose a reason for hiding this comment

akarmi commented Apr 13, 2021

psunn commented Apr 13, 2021

daverim Apr 19, 2021

Choose a reason for hiding this comment

daverim Apr 19, 2021

Choose a reason for hiding this comment

daverim Apr 19, 2021

Choose a reason for hiding this comment

wwwind commented Apr 19, 2021

daverim commented May 10, 2021

google-cla bot commented May 10, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented May 10, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

wwwind commented May 10, 2021

googlebot commented May 10, 2021

google-cla bot commented May 10, 2021

psunn commented May 10, 2021 • edited

wwwind commented May 10, 2021

googlebot commented May 10, 2021

google-cla bot commented May 10, 2021

wwwind commented Jan 5, 2021 •

edited

psunn commented May 10, 2021 •

edited