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 keras-model.ipynb to ./doc/examples #84

Merged
merged 2 commits into from
Apr 3, 2019

Conversation

NickleDave
Copy link
Contributor

No description provided.

@drasmuss
Copy link
Member

drasmuss commented Mar 5, 2019

This looks great, thanks very much for putting it together!

If it's alright with you, I'll add some edits to combine this and https://www.nengo.ai/nengo-dl/examples/pretrained-model.html into the same example. I think it'd be a nice story "here's how you could add a tf-slim model to nengo-dl, and here's how you'd add a keras model to nengo-dl".

@NickleDave
Copy link
Contributor Author

Yes, definitely alright with me. That will probably help with consistency of tone in the docs too.

@drasmuss
Copy link
Member

drasmuss commented Mar 6, 2019

Added a commit merging those two examples. I ended up putting the Keras example first, since I think it's a better introduction (there's more weirdness associated with setting up the tf-slim TensorNode). Let me know if that looks good to you, or feel free to suggest any changes (or just make the edits directly).

The only other thing to do before merging is that we need you to sign the contributor agreement, which is done by opening a PR adding your name to this file https://github.com/nengo/nengo.github.io/blob/src/people.rst.

Thanks again for your work on this, can definitely say after going through the example in detail that this is a nice addition to the documentation.

@drasmuss
Copy link
Member

Just a gentle ping on the status of this @NickleDave. No worries at all if you haven't had a chance to look at it yet, I just wanted to make sure you weren't waiting on me.

@NickleDave
Copy link
Contributor Author

Will look at it tonight! Sorry for the hold-up, it's been on my to-do list, promise

@NickleDave
Copy link
Contributor Author

Just read through it. LGTM! I wouldn't change a thing.

I fetched upstream and rebased on master, hope that didn't mess everything up.

Will submit a separate PR for the contributor agreement

@NickleDave
Copy link
Contributor Author

NickleDave commented Mar 26, 2019

Oh, actually, one thing.
Do you think it would be a good idea to put a link to the license for the Tensorflow docs since I adapted the example from there? Maybe right after mentioning "from the Tensorflow docs"?

I can insert if if you think that's appropriate / necessary.
https://github.com/tensorflow/docs/blob/master/LICENSE

@drasmuss drasmuss force-pushed the add-keras-model-to-examples branch 2 times, most recently from d10f6b1 to 6dfaeec Compare April 3, 2019 19:45
Rename pretrained-model.ipynb to tensorflow-models.ipynb
@drasmuss drasmuss force-pushed the add-keras-model-to-examples branch from 6dfaeec to c15da2c Compare April 3, 2019 19:58
Copy link
Member

@drasmuss drasmuss left a comment

Choose a reason for hiding this comment

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

Cleaned up the history and added a changelog entry. I didn't add a link to the docs license, since I figured that we do already refer to the general TensorFlow license on the LICENSE.rst page (and although the tensorflow/docs license is technically separate from tensorflow/tensorflow, they are the same license so I don't think we need to be too worried about that). I'll merge this in now, thanks again!

@drasmuss drasmuss merged commit c15da2c into nengo:master Apr 3, 2019
@NickleDave
Copy link
Contributor Author

Great! Happy to contribute, and thank you for walking me through the process

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

Successfully merging this pull request may close these issues.

2 participants