Skip to content

Conversation

arovir01
Copy link
Contributor

This PR removes tf.keras.layers.DepthwiseConv2D from the ClusteringRegistry, as our experimental results suggest that depthwise convolution layers are not suited for clustering.

@googlebot googlebot added the cla: yes PR contributor has signed CLA label Jun 24, 2020
@arovir01
Copy link
Contributor Author

arovir01 commented Jun 24, 2020

@Ruomei @akarmi Please review.

@arovir01 arovir01 force-pushed the toupstream/remove_depthwise_conv2d branch from c07975e to e8bc853 Compare June 24, 2020 13:11
@arovir01 arovir01 force-pushed the toupstream/remove_depthwise_conv2d branch from e8bc853 to cd7dc5b Compare June 24, 2020 13:13
@Ruomei
Copy link
Contributor

Ruomei commented Jun 24, 2020

Since this is already unit tested, please let me know whether it works for mobilenet tests. Also, in the model-optimization repo, try "grep" DepthwiseConv2D or lower cases to see whether there are any other places that need to be cleaned up. For instance, in RFC, there is this "except DepthwiseConv2D layers"? do we want to keep it?

@akarmi
Copy link
Contributor

akarmi commented Jun 25, 2020

Since this is already unit tested, please let me know whether it works for mobilenet tests. Also, in the model-optimization repo, try "grep" DepthwiseConv2D or lower cases to see whether there are any other places that need to be cleaned up. For instance, in RFC, there is this "except DepthwiseConv2D layers"? do we want to keep it?

I would argue that we should keep the mentioning of DepthwiseConv2D in the RFC, as this makes it clear that not all layers with potentially clusterable parameters (weights) are actually clustered. Even more, this PR arguably makes the description of clustering configuraiton in that table in RFC more accurate, as it enables appying cluster_weights() to the whole model, and not per-layer, to reproduce the results in those particular rows.

@akarmi
Copy link
Contributor

akarmi commented Jun 29, 2020

@alanchiao, please have a quick look. We decided to disable clustering DepthwiseConv2D layers by default as we have not seen any cases when it works well so far.

@alanchiao
Copy link

Seems fine to me. Once weight clustering is launched, these types of changes will be harder to make when considering backward compatibility.

While I wasn't there for the specific CL discussion (and don't know if how intentional it was), pruning also disables DepthwiseConv2D by default.

@haozha111 haozha111 added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Jun 30, 2020
@copybara-service copybara-service bot merged commit d0ef35e into tensorflow:master Jun 30, 2020
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 ready to pull Working to get PR submitted to internal repository, after which merging to Github happens.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants