Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Update README with Sequential API #648

Closed
wants to merge 7 commits into from
Closed

Update README with Sequential API #648

wants to merge 7 commits into from

Conversation

rickwierenga
Copy link
Contributor

This also shows commits of an earlier PR for some reason.

@Shashi456
Copy link
Contributor

IMO, I think showcasing the common way of declaring models is still the better choice. Sequential exists for people who want to use it, but we still mostly declare models by extending the layer protocol generally.

@pschuh
Copy link
Contributor

pschuh commented Jan 29, 2020

The older PR's changes are due to the previous PR being squashed and merged. You can do a git rebase in order to make your history clean.

@saeta
Copy link
Contributor

saeta commented Jan 29, 2020

Hi @rickwierenga ! First of all, thank you so much for this PR. Documentation is super important and I really appreciate you putting in this effort.

I would like our "front-door" README.md to document the "canonical" way to define layers, as this is where people start in their introduction. Because Sequential is fancy / magical syntactic sugar, it might give people the wrong starting point. (Sequential cannot be the canonical way to write layers, because it does not support skip connections, and other sorts of advanced structure.) That said, it'd be great to document Sequential better, and it looks like there's room for improvement. Do you think you could instead turn this change into an example added onto the Sequential type? (Note: you'll want to add it to the .gyb file so your improvements won't be lost.)

To simplify history, I'm going to close this PR, and look forward to your new PR! (Feel free to reference this PR in your new PR.)

All the best,
-Brennan

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

Successfully merging this pull request may close these issues.

4 participants