Skip to content

Conversation

wwwind
Copy link
Contributor

@wwwind wwwind commented Sep 25, 2020

Small tidy up: removed variables that are not set and not used - some leftovers.

…trainable_weights has all we need.

Change-Id: I271319fd9ff09f71df3178bf2459bde55e09b72c
@googlebot googlebot added the cla: yes PR contributor has signed CLA label Sep 25, 2020
@wwwind wwwind requested a review from akarmi September 25, 2020 09:47
@github-actions github-actions bot added the technique:clustering Regarding tfmot.clustering.keras APIs and docs label Sep 25, 2020
@akarmi akarmi added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Sep 25, 2020

@property
def trainable_weights(self):
return self.layer.trainable_weights + self._trainable_weights
Copy link

@alanchiao alanchiao Sep 28, 2020

Choose a reason for hiding this comment

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

Maybe I've misunderstood but self._trainable_weights and self._non_trainable_weights should be updated automatically during calls to self.add_weight().

By removing "self._trainable_weights" here, someone calling layer.trainable_weights will no longer see all the weights that your wrapper is adding, which breaks the interface.

You can try creating a unit test that checks what layer.trainable_weights outputs and see if it makes sense and look at the base tf.keras.layers.Layer implementation to see what it does.

@alanchiao
Copy link

Marking as not ready to pull given my above comment. Please do add back if it changes.

@alanchiao alanchiao removed the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Oct 5, 2020
@copybara-service copybara-service bot merged commit d1a23df into tensorflow:master Oct 5, 2020
@alanchiao
Copy link

This was accidentally merged - please followup on the above.

@wwwind
Copy link
Contributor Author

wwwind commented Oct 6, 2020

Hi @alanchiao I will investigate. During debugging I have not seen them updating, but I will re-test this.

@benkli01
Copy link
Contributor

Hi @alanchiao and @wwwind ,
as I have encountered some problems with this, I will raise a PR to revert this commit.

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

Labels

cla: yes PR contributor has signed CLA technique:clustering Regarding tfmot.clustering.keras APIs and docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants