Skip to content

[Clustering] Adding a unit test to check all needed weights are returned by cluster_wrapper. #597

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
Merged
merged 1 commit into from
Apr 13, 2021
Merged

[Clustering] Adding a unit test to check all needed weights are returned by cluster_wrapper. #597

merged 1 commit into from
Apr 13, 2021
+37 −53

Conversation

johan-gras
Copy link
Contributor

@johan-gras johan-gras commented Nov 20, 2020 •
edited
Loading

  • Added unit test that make sure that the functions trainable_weights() and non_trainable_weights() in tensorflow_model_optimization/python/core/clustering/keras/cluster_wrapper.py return the correct list of weights (e.g. both weights and centroids).
  • Removed duplicated test: testClusterReassociation.

This is a follow-up PR to #575 based on this comment.

@google-cla google-cla bot added the cla: yes PR contributor has signed CLA label Nov 20, 2020
@github-actions github-actions bot added the technique:clustering Regarding tfmot.clustering.keras APIs and docs label Nov 20, 2020
@wwwind wwwind requested review from Ruomei, akarmi and wwwind and removed request for wwwind November 20, 2020 13:51
Ruomei
Ruomei approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@Ruomei Ruomei left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@wwwind wwwind self-requested a review November 26, 2020 16:53
@wwwind wwwind added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Nov 26, 2020
@google-cla
Copy link

google-cla bot commented Jan 14, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no PR contributor has not signed CLA and removed cla: yes PR contributor has signed CLA labels Jan 14, 2021
@google-cla
Copy link

google-cla bot commented Jan 14, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes PR contributor has signed CLA and removed cla: no PR contributor has not signed CLA labels Jan 14, 2021
@johan-gras
Adding a unit test to check all needed weights are returned by cluste…
4f9e194 
…r_wrapper.

Change-Id: I60b0c901b4d7b699cc44848481518df09d36a97a
@wwwind
Copy link
Contributor

wwwind commented Jan 14, 2021

Hi @daverim This is a small PR that just adds a test and it is ready to be merged.
We had a breakage before, when we refactored trainable_weights() and this test is added to make sure that we don't do this mistake again.
Thanks!

@daverim daverim added ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. and removed ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. labels Jan 19, 2021
daverim
daverim approved these changes Jan 19, 2021
View reviewed changes
Copy link
Collaborator

@daverim daverim left a comment

Choose a reason for hiding this comment

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

Some small nits here, but will fix these on my side

@daverim daverim added ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. and removed ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. labels Mar 30, 2021
@daverim daverim added ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. and removed ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. labels Apr 13, 2021
@copybara-service copybara-service bot merged commit 92486bf into tensorflow:master Apr 13, 2021
liyunlu0618
liyunlu0618 reviewed Apr 13, 2021
View reviewed changes
tensorflow_model_optimization/python/core/clustering/keras/cluster_wrapper_test.py
keras.layers.Dense(8, input_shape=input_shape),
number_of_clusters=2,
cluster_centroids_init=CentroidInitialization.LINEAR
original_layer = cluster_wrapper.ClusterWeights(
Copy link
Contributor

Choose a reason for hiding this comment

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

The test currently fails with this error at line 252:
AttributeError: 'ClusterWeights' object has no attribute 'outputs'

You may want to wrap this in a keras model for the test to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like someone on your side had already fixed this yesterday: https://github.com/tensorflow/model-optimization/blame/master/tensorflow_model_optimization/python/core/clustering/keras/cluster_wrapper_test.py#L246
Should I still update this PR anyway?

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

Reviewers

@daverim daverim daverim approved these changes

@akarmi akarmi Awaiting requested review from akarmi

+3 more reviewers

@liyunlu0618 liyunlu0618 liyunlu0618 left review comments

@wwwind wwwind wwwind approved these changes

@Ruomei Ruomei Ruomei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes PR contributor has signed CLA ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. technique:clustering Regarding tfmot.clustering.keras APIs and docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@johan-gras @googlebot @wwwind @daverim @liyunlu0618 @Ruomei