Skip to content

Conversation

arovir01
Copy link
Contributor

This PR adds the overview document for clustering.

This is still to be viewed as work in progress until the tutorial document will have been finalized.

@googlebot googlebot added the cla: yes PR contributor has signed CLA label Mar 17, 2020
@liufengdb liufengdb requested a review from alanchiao March 19, 2020 05:36
@arovir01 arovir01 force-pushed the toupstream/clustering_guide branch from 566a99c to 4cbe787 Compare April 14, 2020 11:24
@arovir01 arovir01 force-pushed the toupstream/clustering_guide branch from 2396355 to 7ee5c71 Compare May 6, 2020 17:21
Copy link
Contributor

@akarmi akarmi left a comment

Choose a reason for hiding this comment

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

@arovir01, I have spotted a couple of minor things. Please, have a look.

@arovir01 arovir01 force-pushed the toupstream/clustering_guide branch from 2d8ada2 to 0d1b0b4 Compare May 18, 2020 14:09
@alanchiao
Copy link

@lamberta: could you review this PR? This is for a new weight clustering technique that we're moving forward with from this RFC.

@alanchiao alanchiao requested a review from lamberta May 18, 2020 15:51
@arovir01 arovir01 force-pushed the toupstream/clustering_guide branch from 0d1b0b4 to 803798d Compare May 18, 2020 15:59
Copy link
Member

@lamberta lamberta left a comment

Choose a reason for hiding this comment

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

This mentions it's a work in progress, is the intention to add more docs to a /clustering section?

What goes here will publish on tensorflow.org when pulled in. If this shouldn't get published yet, you can prefix the file name with an underscore like: _index.md

Also need to update the leftnav file: https://github.com/tensorflow/model-optimization/blob/master/tensorflow_model_optimization/g3doc/_book.yaml

@alanchiao
Copy link

alanchiao commented May 18, 2020

is the intention to add more docs to a /clustering section?
Yes there is.

If this shouldn't get published yet, you can prefix the file name with an underscore like: _index.md

Yes it's not ready for users to view since the other docs need to be added first. This doc itself is good though. The idea was to not link it into the left nav via _book.yaml so people cannot really view it on tensorflow.org. Only prior to launch, would the _book.yaml changes be made.

If underscore is preferred, we can do that for TFMOT going forward.

@lamberta
Copy link
Member

If underscore is preferred ...

Up to you. If only working in GitHub, I'd probably recommend a feature branch or something and then merge everything when ready to publish. Files prefixed with an underscore are skipped when importing---but that also means the file won't get executed (for testing).

It's fine to publish as normal and then add a nav link when ready, as long as you're not leaking anything, etc. (But if that were the case, we wouldn't be chatting out here in GitHub :)

@alanchiao
Copy link

Ok thanks. Think it's fine to keep the file name as is then.

It matters less for this file, but for the colabs (written by another person), it's nice to be able to get things automatically tested and reviewed document by document.

@arovir01
Copy link
Contributor Author

Also need to update the leftnav file: https://github.com/tensorflow/model-optimization/blob/master/tensorflow_model_optimization/g3doc/_book.yaml

Is this something I should do or will one of you guys handle it after merging this?

@arovir01 arovir01 force-pushed the toupstream/clustering_guide branch from 803798d to 874120b Compare May 19, 2020 13:28
@arovir01
Copy link
Contributor Author

arovir01 commented May 19, 2020

I have added the latest experimental results to the document. Please advise if there is anything else that you consider needs changing. Otherwise I will proceed with squashing the commits together in preparation for merging.

@lamberta
Copy link
Member

@arovir01 for this PR, it sounds like the entry in _book.yaml will be added later by @alanchiao

@alanchiao
Copy link

Billy, yes that's correct. Or Aron / the others will make it shortly prior to launch.

@suharshs suharshs added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Jun 18, 2020
@copybara-service copybara-service bot merged commit 226ca71 into tensorflow:master Jun 25, 2020
@alanchiao
Copy link

Just leaving a note that Billy approved this PR internally.

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