Skip to content
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.

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

Add clustering module to API docs generator #421

Merged

Conversation

arovir01
Copy link
Contributor

@arovir01 arovir01 commented Jun 8, 2020

This PR adds the clustering module to python/core/api and updates build_docs.py to generate the correct API documentation for clustering.

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

@alanchiao Could you please review this?

@alanchiao
Copy link

alanchiao commented Jun 16, 2020

@arovir01: take a look at #427, for what would be better.

Afterwards, we should wait until just a bit prior to launch to submit this PR, since it'll otherwise show up the API docs as launched.

If you want to test the actual imports earlier, we can start by submitting a version of this that

  1. has all the right changes under core/api
  2. under build_docs.py, blacklist everything from the clustering API so that nothing shows up in the API docs.

And then all the colabs can update to their final versions with the import tensorflow_model_optimization as tfmot style imports.

@arovir01 arovir01 force-pushed the toupstream/clustering_docgen branch from 8376dc8 to 1b945f5 Compare June 24, 2020 10:25
@arovir01
Copy link
Contributor Author

@arovir01: take a look at #427, for what would be better.

I removed the wildcard import, as suggested. I will leave the rest as it is until we are ready to sumbit this change.

@arovir01 arovir01 force-pushed the toupstream/clustering_docgen branch 2 times, most recently from 8aa42bf to 4b0e95f Compare July 1, 2020 14:51
@alanchiao
Copy link

alanchiao commented Jul 17, 2020

Looks good. Could you add the additional CentroidInitialization enum? @Ruomei

Keeping it in the same location (as in tfmot.clustering.keras.CentroidInitialization) seems ok in my opinion.

On the side, noticed that CentroidInitialization doesn't have any API docs. https://www.tensorflow.org/api_docs/python/tf/distribute/ReduceOp as an example.
cluster_weights can instead directly reference the CentroidInitialization class in its docs and people can go to
`CentroidInitialization''s API docs to understand the differences between the three (like the references to tf.keras.layers.Layer in https://www.tensorflow.org/api_docs/python/tf/keras/estimator/model_to_estimator).

@arovir01 arovir01 force-pushed the toupstream/clustering_docgen branch from 4b0e95f to dcb5f92 Compare July 17, 2020 10:24
@arovir01
Copy link
Contributor Author

Could you add the additional CentroidInitialization enum?

Done.

On the side, noticed that CentroidInitialization doesn't have any API docs. https://www.tensorflow.org/api_docs/python/tf/distribute/ReduceOp as an example.
cluster_weights can instead directly reference the CentroidInitialization class in its docs and people can go to
`CentroidInitialization''s API docs to understand the differences between the three (like the references to tf.keras.layers.Layer in https://www.tensorflow.org/api_docs/python/tf/keras/estimator/model_to_estimator).

Good point. Will do this in a separate PR, as it is a somewhat distinct piece of work.

@Ruomei
Copy link
Contributor

Ruomei commented Jul 17, 2020 via email

@arovir01
Copy link
Contributor Author

Will do this in a separate PR, as it is a somewhat distinct piece of work.

PR created.

@akarmi
Copy link
Contributor

akarmi commented Jul 20, 2020

@alanchiao, can you please review the recent changes?

@alanchiao
Copy link

Looks good - though it is missing changes to python/core/api/BUILD.

Documentation is under final review so will go ahead and merge this while making the necessary changes to BUILD.

@alanchiao alanchiao added the ready to pull Working to get PR submitted to internal repository, after which merging to Github happens. label Jul 20, 2020
@arovir01
Copy link
Contributor Author

arovir01 commented Jul 20, 2020

it is missing changes to python/core/api/BUILD

Noted. I will add them. Or do we want to do them in a separate change?

@copybara-service copybara-service bot merged commit 15e730a into tensorflow:master Jul 20, 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.

None yet

5 participants