Rewrite of WF lecture #156

Nosferican · 2018-10-05T06:46:12Z

First major re-write. Feedback is welcomed.

jstac · 2018-10-05T10:18:51Z

@Nosferican I think this looks like a big improvement. I defer to others in terms of knowledge of correct Julia style but to me this looks great. It's clear, readable, concise and neat.

Thanks!

sglyon

@Nosferican this looks fantastic!

I have a few questions/comments throughout that I think we should address.

One other question is if we have a build infrastructure in place to build the site for each PR as part of our CI and make the built version visible so reviewers can see the final product?

I've been a bit detached from the build tech for a little while so I'm not sure if I'm asking for something totally unreasonable or perhaps we already have it??

rst_files/wald_friedman.rst

sglyon · 2018-10-05T12:09:58Z

rst_files/wald_friedman.rst


+    plot(plot(reshape(Beta.(1:2:3, 1:2:3), 2, 1), labels = ["f_0", "f_1"], title = "Original Distributions"),
+         plot(reshape(MixtureModel.(Beta, Ref((p -> (p, p)).(1:2:3)), (p -> [p, one(p) - p]).(0.25:0.25:0.75)), 3, 1),


This code is clever!... maybe a bit too cleverl

Does the syntax of broadcasting over an anonymous function (e.g. (p -> [p, one(p) - p]).(0.25:0.25:0.75))) have the potential to scare some of our target audience?

I think we can keep the cleverness, but maybe open up the terseness a little. Something like the following? Also fixes an issue where both subplots had the same metadata.

dists = [Beta(1., 1.), Beta(3., 3.)] # Raw distributions. mix(dists, coef1) = MixtureModel(Beta, params.(dists), [coef1, 1-coef1]) # To generate our mixture models. mixes = mix.(Ref(dists), [0.25, 0.5, 0.75]) plot( plot(dists, labels = ["f_0", "f_1"], title = "Original Distributions"), plot(mixes, labels = ["1/4-3/4", "1/2-1/2", "3/4-1/4"], title = "Distribution Mixtures"), ylab = "Density", ylim = (0, 2), layout = (2, 1) # Global settings across both plots )

Yes, I never like clever. Terse is not necessarily a virtue and don't forget point 1 of https://github.com/QuantEcon/lecture-source-jl/blob/master/style.md#basic-principles

I think @arnavs has provided something which we can teach a little easier.

I strive to be, hehe. I took from the @arnavs version and made it a bit more slow to help the reader follow.

Run You Clever Boy, and Remember. - Clara Oswald

sglyon · 2018-10-05T12:12:09Z

rst_files/wald_friedman.rst

-.. math::
-
-    J(p) = J(p_i) + (p - p_i) \frac{J(p_{i+1}) - J(p_i)}{p_{i+1} - p_{i}}
+We implement the cost functions for each choice considered in the


This is a break from writing the style adopted in most of the lectures.

In most places on lectures.quantecon.org we have one sentence paragraphs without punctuation at the end. Here we have replaced that with more conventional paragraphs that have multiple sentences separated by punctuation marks.

I'm not casting a vote here for which I think we should have going forward, my purpose is only to point out that the change occurred

This applies to a few places in the diff, but I'll only mention it once

Right. Let's stick to one sentence lines with a line break after and no full stop.

That's something we can change later but let's at least be uniform.

Good catch. I have modified it to be consistent with that part of the style.

jstac · 2018-10-05T12:29:46Z

@mmcky Can answer your questions on the build infrastructure, @sglyon

arnavs · 2018-10-05T16:38:14Z

@sglyon Specifically to the CI, we're looking into a Docker-based solution. Something like https://docs.travis-ci.com/user/docker/? I think @jlperla wrote an issue on this, but I can't find it.

rst_files/wald_friedman.rst

arnavs · 2018-10-05T17:04:09Z

rst_files/wald_friedman.rst


+    plot(plot(reshape(Beta.(1:2:3, 1:2:3), 2, 1), labels = ["f_0", "f_1"], title = "Original Distributions"),
+         plot(reshape(MixtureModel.(Beta, Ref((p -> (p, p)).(1:2:3)), (p -> [p, one(p) - p]).(0.25:0.25:0.75)), 3, 1),


I think we can keep the cleverness, but maybe open up the terseness a little. Something like the following? Also fixes an issue where both subplots had the same metadata.

dists = [Beta(1., 1.), Beta(3., 3.)] # Raw distributions. mix(dists, coef1) = MixtureModel(Beta, params.(dists), [coef1, 1-coef1]) # To generate our mixture models. mixes = mix.(Ref(dists), [0.25, 0.5, 0.75]) plot( plot(dists, labels = ["f_0", "f_1"], title = "Original Distributions"), plot(mixes, labels = ["1/4-3/4", "1/2-1/2", "3/4-1/4"], title = "Distribution Mixtures"), ylab = "Density", ylim = (0, 2), layout = (2, 1) # Global settings across both plots )

arnavs · 2018-10-05T17:06:09Z

rst_files/wald_friedman.rst

-        EJ = dot(curr_dist, J.(tp1_dist))
-
-        return EJ
+    accept_x0(p, L0) = (one(p) - p) * L0


I think these functions are excellent. Very close to the bare math.

arnavs · 2018-10-05T17:08:52Z

rst_files/wald_friedman.rst


-        return correct, p, t
+    @with_kw struct Problem


Is there a reason we're not using a NamedTuple? Like:

Problem = @with_kw (d0 = Beta(1,1), d1 = Beta(9,9), L0 = 2, L1 = 2, c = 0.2, p = 0.5, n = 100, return_output = false) # Constructor for problem objects. DefaultProblem = Problem() ModifiedProblem = Problem(d0 = Beta(1, 3))

Yes: https://github.com/QuantEcon/lecture-source-jl/blob/master/style.md#type-annotations-parameters-and-generic-programming

We want to avoid these kinds of structs at all costs. Not to mention that this struct is not templatized for the distributions, so it would be holding abstract types.

Luckily, named tuples do it all.

Good catch. I have made the changes to use NamedTuple.

jlperla · 2018-10-05T17:26:40Z

There are improvements here, but I strongly feel we should avoid rewriting these complicated lectures and focus on the many, many simple ones that exist.

The patterns for things like dealing with fixed points and dynamic programming in the simple examples have not been established. My fear is that starting here makes us go from a clean functional-style code that matches the math, and move towards terse, self-contained, and clever code with lots of for and while loops.

So my feeling is: merge this with minor changes, and then go back, starting with the simplest examples and working up.

jlperla · 2018-10-05T17:31:19Z

rst_files/wald_friedman.rst

+        trials = fill(0, n)
+        for trial in 1:n
+            truth = rand(1:2)
+            d = (d0, d1)[truth]


This is very terse syntax. I don't think that this is going to be easy for people to read.

Used some comments to help read the code better.

jlperla · 2018-10-05T17:33:36Z

rst_files/wald_friedman.rst

+            p = bayes_update(p, d0, d1)
+            cost += c
+            candidate = min(accept_x0(p, L0), accept_x1(p, L1)) + cost
+            candidate ≥ target && break


https://github.com/QuantEcon/lecture-source-jl/blob/master/style.md#naming-conventions-comments-etc ascii for control flow

jlperla · 2018-10-05T17:34:40Z

rst_files/wald_friedman.rst

-            J_out[p_ind] = min(p_c_0, p_c_1, p_con)
+    function decision_rule(d0, d1, L0, L1, c)
+        function cost(p, d0, d1, L0, L1, c)
+            c ≥ zero(c) || throw(ArgumentError("Cost must be non-negative"))


https://github.com/QuantEcon/lecture-source-jl/blob/master/style.md#general-control-structures-and-code-organization Avoid the short-circuiting pattern when you just want to do an assert.

These is very tough to read for an introductory user.

jlperla · 2018-10-05T17:35:25Z

rst_files/wald_friedman.rst

-                decision = 0
-                break
+        for (left, right, root) in zip(reverse(left), reverse(right), reverse(roots))
+            if left ≠ 1 && right == 1


Use ascii for control flow

Replace && with &? Or ≠ with !=? ≠ is closer to the math.
I saw the style guide had != over ≠, but ≠ was in the allowed unicode guide. I don't think there is much sense to use ≠ outside of control flow.

Those the style also mean I should change ≥/≤ for control flow? Those are the other non-ASCII used in control flow in this lectures I believe.

Yeah, I think that is right. Though I really do love the unicode for ≠ if we are writing up a line of math where it is used for set inclusion, etc.

For set inclusion it would be ∈ or ∉. I don't think ≠ is used outside control flow or tests... Very few exceptions is any. Probably best to discuss it later on the style guide, but I would cast my vote towards keeping control flow operations close to the math as well.

Of course, you are right. Then I don't think there is any reason to use ≠ except when doing DSL-y type stuff

Control flow is part of an algorithm, and not a direct implementation of a mathematical formula in the lecture notes. Making the distinction between the direct implementation of the formulas from the math and the control flow of algorithms is a key goal.

jlperla · 2018-10-05T17:37:07Z

rst_files/wald_friedman.rst

-            elseif p > ub
-                decision = 0
-                break
+        for (left, right, root) in zip(reverse(left), reverse(right), reverse(roots))


Ascii for control flow. But I think that a comment could help a lot to explain what is going on here.

Where is non-ASCII being used for control flow?

Thanks.

Nosferican · 2018-10-18T20:06:26Z

@arnavs Final check and and if all good, good to merge.

arnavs · 2018-10-18T20:12:33Z

@Nosferican Sounds good. Will review and merge after class/meetings.

Re write of WF lecture

2b17522

First major re-write. Feedback is welcomed.

Nosferican added the enhancement label Oct 5, 2018

Nosferican self-assigned this Oct 5, 2018

Nosferican requested review from arnavs and jlperla October 5, 2018 06:46

sglyon suggested changes Oct 5, 2018

View reviewed changes

arnavs previously requested changes Oct 5, 2018

View reviewed changes

arnavs mentioned this pull request Oct 5, 2018

Structs => NamedTuples: "A Problem that Stumped Milton Friedman" #151

Closed

jlperla reviewed Oct 5, 2018

View reviewed changes

Addressed comments

582b224

Nosferican mentioned this pull request Oct 6, 2018

Re-writes Tracker #160

Closed

53 tasks

Merge branch 'master' into WF_Re-write

0a9971a

Nosferican merged commit d819cd3 into master Oct 20, 2018

arnavs deleted the WF_Re-write branch December 14, 2018 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of WF lecture #156

Rewrite of WF lecture #156

Nosferican commented Oct 5, 2018

jstac commented Oct 5, 2018

sglyon left a comment

sglyon Oct 5, 2018

arnavs Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018 •

edited

Loading

sglyon Oct 5, 2018

jstac Oct 5, 2018

Nosferican Oct 5, 2018

jstac commented Oct 5, 2018

arnavs commented Oct 5, 2018

arnavs Oct 5, 2018

arnavs Oct 5, 2018

arnavs Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018

jlperla commented Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018

jlperla Oct 5, 2018

jlperla Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018 •

edited

Loading

Nosferican Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018

jlperla Oct 5, 2018

jlperla Oct 5, 2018

Nosferican Oct 5, 2018

Nosferican commented Oct 18, 2018

arnavs commented Oct 18, 2018


		plot(plot(reshape(Beta.(1:2:3, 1:2:3), 2, 1), labels = ["f_0", "f_1"], title = "Original Distributions"),
		plot(reshape(MixtureModel.(Beta, Ref((p -> (p, p)).(1:2:3)), (p -> [p, one(p) - p]).(0.25:0.25:0.75)), 3, 1),

Rewrite of WF lecture #156

Rewrite of WF lecture #156

Conversation

Nosferican commented Oct 5, 2018

jstac commented Oct 5, 2018

sglyon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nosferican Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstac commented Oct 5, 2018

arnavs commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlperla commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nosferican Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nosferican commented Oct 18, 2018

arnavs commented Oct 18, 2018

Nosferican Oct 5, 2018 •

edited

Loading

Nosferican Oct 5, 2018 •

edited

Loading