Issue 4891041: code review 4891041: doc/codewalk: new Markov chain codewalk (Closed)
|
Created:
13 years, 8 months ago by adg Modified:
13 years, 8 months ago Reviewers:
CC:
gri, r, rsc, golang-dev Visibility:
Public. |
Descriptiondoc/codewalk: new Markov chain codewalk
Patch Set 1 #Patch Set 2 : diff -r b0680f902518 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r de51e6f7dd3e https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 65e4ed2fa573 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 65e4ed2fa573 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 65e4ed2fa573 https://go.googlecode.com/hg/ #
Total comments: 18
Patch Set 7 : diff -r 53b068e61dd0 https://go.googlecode.com/hg/ #
Total comments: 76
Patch Set 8 : diff -r 83b22ae0f3e8 https://go.googlecode.com/hg/ #
Total comments: 10
Patch Set 9 : diff -r 245a17ebba26 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 10 : diff -r 1e3e5c27e0a3 https://go.googlecode.com/hg/ #MessagesTotal messages: 10
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:16: Our Markov chain algorthim would arrange this text into this set of prefixes s/algorthim/algorithm/ http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:37: limit. (The word limit is necesasry as the chain table may contain cycles.) s/necesasry/necessary/ http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:55: // Prefix is an n-word Markov chain prefix. where is the n coming in here? Is it the length of Prefix? should the comment be updated? http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:63: // Rotate removes the first word from the Prefix and appends the given suffix. suffix must be word here, I assume. perhaps clarify the comment. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:71: chain map[string][]string the prefix is the concatenated words, while the suffix is a list of words? perhaps clarify in the comment http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:101: <code><a href="/pkg/fmt/#Fscan">fmt.Fscan</a></code> function which I think Fscan's result should be described a bit better ("reads values") doesn't say much. I'd need to look it up. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:145: For more about the <code>append</code> function and silces in general For more information about... (?) http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:270: And then execute it piping in some input text. ...execute it by piping in ... (?) http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:271: Here's a demo of generating text using the Go distribution's s/generating/generated/ (?)
Sign in to reply to this message.
Thanks. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:16: Our Markov chain algorthim would arrange this text into this set of prefixes On 2011/08/15 23:10:47, gri wrote: > s/algorthim/algorithm/ Done. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:37: limit. (The word limit is necesasry as the chain table may contain cycles.) On 2011/08/15 23:10:47, gri wrote: > s/necesasry/necessary/ Done. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:55: // Prefix is an n-word Markov chain prefix. On 2011/08/15 23:10:47, gri wrote: > where is the n coming in here? Is it the length of Prefix? should the comment be > updated? It's just a bad comment. Rewritten to // Prefix is a Markov chain prefix of one or more words. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:63: // Rotate removes the first word from the Prefix and appends the given suffix. On 2011/08/15 23:10:47, gri wrote: > suffix must be word here, I assume. perhaps clarify the comment. It refers to the suffix argument, but I think word is a better name for the argument. Changed. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:71: chain map[string][]string On 2011/08/15 23:10:47, gri wrote: > the prefix is the concatenated words, while the suffix is a list of words? > perhaps clarify in the comment Done. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:101: <code><a href="/pkg/fmt/#Fscan">fmt.Fscan</a></code> function which On 2011/08/15 23:10:47, gri wrote: > I think Fscan's result should be described a bit better ("reads values") doesn't > say much. I'd need to look it up. How about "reads space-separated values"? There's more discussion of Fscan in another couple of sections. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:145: For more about the <code>append</code> function and silces in general On 2011/08/15 23:10:47, gri wrote: > For more information about... (?) Done. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:270: And then execute it piping in some input text. On 2011/08/15 23:10:47, gri wrote: > ...execute it by piping in ... (?) "while" makes more sense. http://codereview.appspot.com/4891041/diff/13001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:271: Here's a demo of generating text using the Go distribution's On 2011/08/15 23:10:47, gri wrote: > s/generating/generated/ (?) Added some other words.
Sign in to reply to this message.
http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newcode6 doc/codewalk/markov.go:6: Generating random text; a Markov chain algorithm s/;/:/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newcode9 doc/codewalk/markov.go:9: of The Practice of Programming (Kernighan and Pike, Addison-Wesley 1999). See also Computer Recreations, Scientific American 260, 122 - 125 (1989). http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:14: I am not an animal. I am a human being. I am not a number! I am a free man! is more cultured a reference (and also more nearly correct). http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:33: example), choose one of the suffixes associated with that prefix at random s/at random/&, with probability determined by the input statistics,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:39: This program reads text from standard input, parsing it into a Markov chain, s/This program/Our version of the program/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:66: p[len(p)-1] = suffix p[copy(p, p[1:]] = suffix may be too cute, but it's correct http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:70: // A prefix is a string of prefixLen words concatenated with spaces. s/concatenated/joined/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:114: numWords = flag.Int("words", 100, "maximum length of output in words") maximum number of words to print http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:125: fmt.Println(text) // Write text to standard output. this is allocation-heavy. it is better style to have Generate take a writer, but not as cute. mention this as an exercise. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:15: <step title="Modelling Markov chains" src="doc/codewalk/markov.go:/ chain/"> s/ll/l/ for american spelling http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:35: "not an": {"animal."}, use new example http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:63: do not begin with an uppercase character), and so we write a upper case http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:68: This is not strictly necessary as this entire program is within a what's "This"? http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:80: This allows us to define methods that operate on <code>Prefix</code> again with This. it's sloppy writing http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:82: a <code>[]string</code>. point out that this is Go and we get to define method on slice http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:101: <code><a href="/pkg/fmt/#Fscan">fmt.Fscan</a></code> function which s/function/&,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:120: <code>fmt.Fscan</code>. Since <code>Fscan</code> uses whitespace as a Since Fscan uses space to separate each input value, http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:138: <code>nil</code> <code>append</code> allocates a new slice. nil, append ... http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:139: This conveniently ties in with the semantics of our map: retrieving This behavior http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:142: This way we can use <code>append</code> to allocate new slices for jeez http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:145: For more information about the <code>append</code> function and silces slices http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:162: functionality inside a method on <code>Prefix</code> named two this's. the first is fine, the second not so clear. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:169: the start of the slice. This has the effect of moving the elements this http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:175: Then it assigns the provided <code>word</code> to the last index not a sentence http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:189: Instead of looping indefinitely like <code>Build</code>, weird detail. I'd drop the comparison and just say what it does http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:197: <code>p.String()</code>, and assign its contents to <code>slist</code>. s/,// http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:216: Then we <code>Rotate</code> the new suffix onto the prefix just like Next, we ... s/like/as/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:223: the <code>words</code> slice together separated by spaces. concatenate...separated is weird. join...separated is not s/together/&,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:227: To make it easy to tweak the prefix and generated text lengths we'll s/we'll/we/ stay in the present tense http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:244: If the command line flags provided by the user are invalid the ,s/command line flags/command-line flags/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:250: To create the new <code>Chain</code> we call <code>NewChain</code> s/$/,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:251: passing in the value of the <code>prefix</code> flag. s/passing in/passing/ or /with/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:253: To build the chain we call <code>Build</code> and pass in s/and pass in/with/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:259: Finally, to generate text we call <code>Generate</code> passing in s/ passing in/, with/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:260: the value of the <code>words</code> flag and assinging the result assigning http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:264: output followed by a carriage return. s/output/&,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:268: To use this program, first compile and link it. For example: s/For example:/If you're using 6g as your compiler, the command would be something like,/ http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:271: And then execute it while piping in some input text. where is the execution command?
Sign in to reply to this message.
PTAL http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newcode6 doc/codewalk/markov.go:6: Generating random text; a Markov chain algorithm On 2011/08/16 06:16:41, r wrote: > s/;/:/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newcode9 doc/codewalk/markov.go:9: of The Practice of Programming (Kernighan and Pike, Addison-Wesley 1999). On 2011/08/16 06:16:41, r wrote: > See also Computer Recreations, Scientific American 260, 122 - 125 (1989). Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:14: I am not an animal. I am a human being. On 2011/08/16 06:16:41, r wrote: > I am not a number! I am a free man! > > is more cultured a reference (and also more nearly correct). Good call. I like McGoohan way more than Heston anyway. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:33: example), choose one of the suffixes associated with that prefix at random On 2011/08/16 06:16:41, r wrote: > s/at random/&, with probability determined by the input statistics,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:39: This program reads text from standard input, parsing it into a Markov chain, On 2011/08/16 06:16:41, r wrote: > s/This program/Our version of the program/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:66: p[len(p)-1] = suffix On 2011/08/16 06:16:41, r wrote: > p[copy(p, p[1:]] = suffix > > may be too cute, but it's correct Kinda conflates the two separate steps, though. I'll leave it as-is. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:70: // A prefix is a string of prefixLen words concatenated with spaces. On 2011/08/16 06:16:41, r wrote: > s/concatenated/joined/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:114: numWords = flag.Int("words", 100, "maximum length of output in words") On 2011/08/16 06:16:41, r wrote: > maximum number of words to print Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:125: fmt.Println(text) // Write text to standard output. On 2011/08/16 06:16:41, r wrote: > this is allocation-heavy. it is better style to have Generate take a writer, but > not as cute. mention this as an exercise. Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:15: <step title="Modelling Markov chains" src="doc/codewalk/markov.go:/ chain/"> On 2011/08/16 06:16:41, r wrote: > s/ll/l/ for american spelling Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:35: "not an": {"animal."}, On 2011/08/16 06:16:41, r wrote: > use new example Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:63: do not begin with an uppercase character), and so we write a On 2011/08/16 06:16:41, r wrote: > upper case Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:68: This is not strictly necessary as this entire program is within a On 2011/08/16 06:16:41, r wrote: > what's "This"? Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:80: This allows us to define methods that operate on <code>Prefix</code> On 2011/08/16 06:16:41, r wrote: > again with This. it's sloppy writing Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:82: a <code>[]string</code>. On 2011/08/16 06:16:41, r wrote: > point out that this is Go and we get to define method on slice Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:101: <code><a href="/pkg/fmt/#Fscan">fmt.Fscan</a></code> function which On 2011/08/16 06:16:41, r wrote: > s/function/&,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:120: <code>fmt.Fscan</code>. Since <code>Fscan</code> uses whitespace as a On 2011/08/16 06:16:41, r wrote: > Since Fscan uses space to separate each input value, Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:138: <code>nil</code> <code>append</code> allocates a new slice. On 2011/08/16 06:16:41, r wrote: > nil, append ... Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:139: This conveniently ties in with the semantics of our map: retrieving On 2011/08/16 06:16:41, r wrote: > This behavior Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:142: This way we can use <code>append</code> to allocate new slices for On 2011/08/16 06:16:41, r wrote: > jeez Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:145: For more information about the <code>append</code> function and silces On 2011/08/16 06:16:41, r wrote: > slices Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:162: functionality inside a method on <code>Prefix</code> named On 2011/08/16 06:16:41, r wrote: > two this's. the first is fine, the second not so clear. Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:169: the start of the slice. This has the effect of moving the elements On 2011/08/16 06:16:41, r wrote: > this Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:175: Then it assigns the provided <code>word</code> to the last index On 2011/08/16 06:16:41, r wrote: > not a sentence Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:189: Instead of looping indefinitely like <code>Build</code>, On 2011/08/16 06:16:41, r wrote: > weird detail. I'd drop the comparison and just say what it does Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:197: <code>p.String()</code>, and assign its contents to <code>slist</code>. On 2011/08/16 06:16:41, r wrote: > s/,// Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:216: Then we <code>Rotate</code> the new suffix onto the prefix just like On 2011/08/16 06:16:41, r wrote: > Next, we ... > s/like/as/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:223: the <code>words</code> slice together separated by spaces. On 2011/08/16 06:16:41, r wrote: > concatenate...separated is weird. join...separated is not > > s/together/&,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:227: To make it easy to tweak the prefix and generated text lengths we'll On 2011/08/16 06:16:41, r wrote: > s/we'll/we/ > stay in the present tense Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:244: If the command line flags provided by the user are invalid the On 2011/08/16 06:16:41, r wrote: > ,s/command line flags/command-line flags/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:250: To create the new <code>Chain</code> we call <code>NewChain</code> On 2011/08/16 06:16:41, r wrote: > s/$/,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:251: passing in the value of the <code>prefix</code> flag. On 2011/08/16 06:16:41, r wrote: > s/passing in/passing/ or /with/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:253: To build the chain we call <code>Build</code> and pass in On 2011/08/16 06:16:41, r wrote: > s/and pass in/with/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:259: Finally, to generate text we call <code>Generate</code> passing in On 2011/08/16 06:16:41, r wrote: > s/ passing in/, with/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:260: the value of the <code>words</code> flag and assinging the result On 2011/08/16 06:16:41, r wrote: > assigning Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:264: output followed by a carriage return. On 2011/08/16 06:16:41, r wrote: > s/output/&,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:268: To use this program, first compile and link it. For example: On 2011/08/16 06:16:41, r wrote: > s/For example:/If you're using 6g as your compiler, the command would be > something like,/ Done. http://codereview.appspot.com/4891041/diff/17001/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:271: And then execute it while piping in some input text. On 2011/08/16 06:16:41, r wrote: > where is the execution command? Done.
Sign in to reply to this message.
LGTM Very nice. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode22 doc/codewalk/markov.go:22: <nil> <nil> I You say <nil> but the code uses "". http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode66 doc/codewalk/markov.go:66: // Rotate removes the first word from the Prefix and appends the given word. This isn't a rotate. This is a shift. Rotate implies p[len(p)-1 ] = p[0]. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode91 doc/codewalk/markov.go:91: if _, err := fmt.Fscan(r, &s); err != nil { It is surprising to me that this works given that you've passed os.Stdin unbuffered. Occasionally Fscan wants to unread a rune and it cannot on an *os.File. I'd do br := bufio.NewReader(r) at the beginning of this function and scan from br here. It will be more efficient anyway. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcod... doc/codewalk/markov.go:100: // Generate returns at most n words generated from Chain. returns a string of at http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcod... doc/codewalk/markov.go:105: slist, ok := c.chain[p.String()] choices := c.chain[p.String()] if len(choices) == 0 { break } next := choices[rand.Intn(len(choices))] words = append(words, next) p.Shift(next)
Sign in to reply to this message.
Thanks. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode22 doc/codewalk/markov.go:22: <nil> <nil> I On 2011/08/16 20:45:19, rsc wrote: > You say <nil> but the code uses "". Done. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode66 doc/codewalk/markov.go:66: // Rotate removes the first word from the Prefix and appends the given word. On 2011/08/16 20:45:19, rsc wrote: > This isn't a rotate. This is a shift. > Rotate implies p[len(p)-1 ] = p[0]. > Done. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcode91 doc/codewalk/markov.go:91: if _, err := fmt.Fscan(r, &s); err != nil { On 2011/08/16 20:45:19, rsc wrote: > It is surprising to me that this works given that you've > passed os.Stdin unbuffered. Occasionally Fscan wants > to unread a rune and it cannot on an *os.File. > > I'd do > > br := bufio.NewReader(r) > > at the beginning of this function and scan from br here. > It will be more efficient anyway. Done. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcod... doc/codewalk/markov.go:100: // Generate returns at most n words generated from Chain. On 2011/08/16 20:45:19, rsc wrote: > returns a string of at Done. http://codereview.appspot.com/4891041/diff/8002/doc/codewalk/markov.go#newcod... doc/codewalk/markov.go:105: slist, ok := c.chain[p.String()] On 2011/08/16 20:45:19, rsc wrote: > choices := c.chain[p.String()] > if len(choices) == 0 { > break > } > next := choices[rand.Intn(len(choices))] > words = append(words, next) > p.Shift(next) Done.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:120: prefixLen = flag.Int("prefix", 2, "prefix length in words") these should be local to main since they're used only there http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:24: as modelled by this data structure: s/ll/l/ http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:113: This function does many small reads which can be inefficient for some s/reads/&,/
Sign in to reply to this message.
http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.go File doc/codewalk/markov.go (right): http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.go#newco... doc/codewalk/markov.go:120: prefixLen = flag.Int("prefix", 2, "prefix length in words") On 2011/08/17 04:39:19, r wrote: > these should be local to main since they're used only there Done. http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml File doc/codewalk/markov.xml (right): http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:24: as modelled by this data structure: On 2011/08/17 04:39:19, r wrote: > s/ll/l/ Done. http://codereview.appspot.com/4891041/diff/17002/doc/codewalk/markov.xml#newc... doc/codewalk/markov.xml:113: This function does many small reads which can be inefficient for some On 2011/08/17 04:39:19, r wrote: > s/reads/&,/ Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=8780dd3ee36a *** doc/codewalk: new Markov chain codewalk R=gri, r, rsc CC=golang-dev http://codereview.appspot.com/4891041
Sign in to reply to this message.
|
