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

Follow up of PR #5 #6

Merged
merged 13 commits into from
Jul 23, 2021
Merged

Follow up of PR #5 #6

merged 13 commits into from
Jul 23, 2021

Conversation

thevasudevgupta
Copy link
Owner

@thevasudevgupta thevasudevgupta commented Jul 20, 2021

This is the follow up of PR #5.

Major Change:

  • Text is added to the notebook (while code is kept untouched)

@sayakpaul @MorganR

@thevasudevgupta
Copy link
Owner Author

I have submitted PR for exporting saved-model here: tensorflow/tfhub.dev#65

Copy link
Collaborator

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

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

@vasudevgupta7 great work with the notebook. Just scan it through any pesky grammatical or spelling-related bugs. For example, I caught one here (repositary):

Finally, we have reached an end to this notebook. But it's not an end of learning TensorFlow for speech-related tasks, this repositary contains some more amazing tutorials. Feel free to go through them. In case you encounter any bug, please create an issue here.

You might want to also give this repository a shoutout as it shows many non-trivial and relevant TFLite workflows. It's entirely up to you.

Here are some observations:

  • The number of trainable parameters seems off. Should it be just 2? Note that if it's a Keras model you call call count_params() on it. trainable_variables returns a list and in your code length of those lists is being accounted for and NOT the actual elements that are present inside them.
  • Is it possible to operate on a small portion of the dataset and actually show how to preprocess it? It's not mandatory in this case to use TFRecords just so you know. You might also consider outputting a few samples from the training set so that the readers are aware of what they are dealing with.
  • Consider noting about the pre-trained model for at least two sentences to make things more complete.
  • See if we can also add an evaluation metric to report on the datasets.
  • Let's wrap this model as a subclass of tf.keras.Model as well.

Let me know if anything is unclear.

@thevasudevgupta
Copy link
Owner Author

@sayakpaul, @MorganR,

notebook is ready for final review !!

Major changes:

  • worked on all the suggestions in notebook by @sayakpaul
  • hub.KerasLayer now can take the link directly (no need of extra thinking about signature)
  • Added argparse support for conversion script

@thevasudevgupta
Copy link
Owner Author

Merging this PR to be able to access link of the notebook from TFHub. But open for any kind of feedback (will work on that in another PR).

@thevasudevgupta thevasudevgupta merged commit 4889e6f into main Jul 23, 2021
@thevasudevgupta thevasudevgupta deleted the export-v2 branch July 23, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants