[exchangeable] Simplify code and bring lecture into style-guide compliance - #772
Conversation
|
📖 Netlify Preview Ready! Preview URL: https://pr-772--sunny-cactus-210e3e.netlify.app (5098c0e) 📚 Changed Lecture Pages: exchangeable |
|
@longye-tian , would you be willing to review this PR? @mmcky The deploy link seems broken. Do they go stale? after how long? Should @longye-tian trigger a new build with an empty commit? |
|
@longye-tian or any other reviewer: Does adding JAX improve this lecture? Please also make this value judgement. |
There was a problem hiding this comment.
I think the JAX conversion is natural in the appendix simulation section, especially around the replacement of the old nested-loop simulation. This seems like a good fit because the simulation has two parts: recursion over time and repetition across independent paths.
It might be helpful to say this explicitly in the text before the code. Currently the lecture says:
To proceed, we create some Python code.
Perhaps add something like:
The simulation has two dimensions: recursion over time and repetition across independent paths. In JAX, we use
lax.scanfor the time recursion andvmapto apply the path simulator across many independent key sequences.
For the learning_example block, I’m less sure that the JAX conversion adds much. This part changes the beta-density helper from the old Numba/vectorized version and wraps several scalar helpers with jax.jit, but the surrounding computation is still mostly SciPy root finding, SciPy quadrature, NumPy grids, and Matplotlib plotting. So JAX transformations do not seem central to this part of the lecture. I can see the value of removing Numba and using JAX for consistency, but plain NumPy/SciPy may be clearer here.
A more useful exposition improvement might be to split this page-long helper function into the three conceptual pieces shown in the output figure:
- likelihood ratio plot
- density / probability-region plot
- posterior-dynamics arrows
That would let the text introduce and discuss each panel one by one, rather than asking readers to parse a long plotting helper before seeing the three ideas it creates.
A few small code-level updates could also make the JAX appendix clearer:
- Instead of
random_seed = int(a * b + T + N), consider adding an explicitseedorkeyargument. That would make the source of randomness clearer. - The comment
# Generate all random keys upfrontappears insidesimulate_path, but the keys are actually generated insimulate; perhaps change it to something like# Use one random key for each date.
A few small remaining typos I noticed while reading:
- “less that one” → “less than one”
- “Bayes’ Law make
$\pi$ decrease” → “Bayes’ Law makes$\pi$ decrease” - “The above graphs shows” → “The above graph shows”
-
expected_rario→expected_ratio
Best,
Longye
|
🤖 Status note for a future session — from a maintainer investigation on 2026-07-08 into why open-PR previews 404. Context only, not instructions. Netlify preview: https://pr-772--sunny-cactus-210e3e.netlify.app/ currently returns 404. Why previews are down (repo-wide findings)1. This branch is stale — 93 commits behind 2. The arviz failure was a red herring — do NOT pin arviz or rewrite plotting. A 2026-07-07 rebuild also failed in Note on recent timeline activityThis PR was close/reopened on 2026-07-07 by a maintainer session purely to trigger a rebuild test — not a content change. That rebuild failed on the stale-branch issue above. Apologies for the notification churn. Recommended first step for this PRUpdate this branch to This PR touches: |
|
Thanks @longye-tian. I will review your comment and make the changes. |
📖 Netlify Preview Ready!Preview URL: https://pr-772--sunny-cactus-210e3e.netlify.app Commit: 📚 Changed LecturesBuild Info
|
…iance Following review feedback on #772, drop the JAX conversion rather than extend it. Per the JAX style guide, the numerical work here runs through SciPy root finding and quadrature on 100-point plotting grids, so JAX brought its constraints without its benefits (the jitted section measured ~600x slower than plain NumPy, since quad calls the density scalar by scalar). Numba is removed as well: the density needs no jit, and the simulation no longer has a loop to compile. Changes: - Split the page-long learning_example helper into create_model plus three plotting functions, one per panel, so the text can introduce and discuss each graph in turn (Longye's review suggestion). - Derive Bayes' Law in odds form and simulate the belief ensemble as a cumulative product of likelihood ratios, removing the path loop entirely. This is closer to the mathematics and motivates the reference to likelihood ratio processes already in the text. - Use np.random.default_rng with an explicit generator, per the style guide on NumPy random number generation. - Replace four `{doc}`this lecture <...>`` links with the auto-title form, and the hard-coded python-advanced URL with an intersphinx reference. - Lower-case section headings below the lecture title. - Fix typos: "less that one", "makes π decrease", "absciassa", "whcih", "expected_rario", "a sequence is random variables", "about about". Figures are unchanged in shape and magnitude; the three-panel numbers (0.524, 0.816, 0.000749) reproduce exactly. Full lecture runs in ~2.6s versus ~3.4s on main and ~7.8s on the JAX branch.
|
Thanks @kp992 for the work here, and @longye-tian for a careful review — the review is what turned this around, so let me explain where it landed and why. SummaryI've changed direction on this PR: rather than extend the JAX conversion, we drop JAX entirely and keep the lecture in NumPy/SciPy. Numba goes too. I've also acted on Longye's structural suggestion and taken the opportunity to fix some pre-existing style-guide violations. I've pushed this to the branch and retitled the PR accordingly. Why not JAXLongye's instinct about
and
That is exactly the situation in That left only the appendix simulation as a genuine JAX candidate, and that turned out to have a much better answer than either What replaced it1. The simulation is now a cumulative product. Writing Bayes' Law in odds form, so the whole ensemble of belief paths is a 2. 3. Random number generation now uses Style-guide fixesSome of these predate this PR, but they were worth fixing while the file was open:
VerificationAll twelve figures were rendered and compared against Runtime for the whole lecture:
One footnote for anyone reading the diff: I first wrote the Beta density as @kp992, apologies that this reworks your PR fairly heavily — the JAX question was one I asked and it needed a maintainer answer, which it didn't get until now. The typo fixes and the structural cleanup you started are all still here. @longye-tian, thank you — both of your substantive points are now addressed, one by removal rather than by revision. |
|
@kp992 @longye-tian The above report is written by Claude. It's getting much easier to determine what should go to JAX and what should stay -- just point Claude at the style guide (manual) and ask it to assess the value of the conversion based on the benchmarks in that style guide. In this case Claude finds that there's no real value in shifting to JAX so I'm reverting to NumPy. I'm following @longye-tian 's suggestion for improving readability and also taking the opportunity to bring the rest of the lecture in line with the style guide. Merging this now (CC @mmcky). |
✅ Translation sync completed (zh-cn)Target repo: QuantEcon/lecture-python.zh-cn
|
Update the lecture to use JAX and removes numba related code. Also fixes minor typos and code styling issues.