[imp_sample] Convert code to JAX and check stylesheet compliance - #620
Conversation
|
📖 Netlify Preview Ready! Preview URL: https://pr-620--sunny-cactus-210e3e.netlify.app (eba931b) 📚 Changed Lecture Pages: imp_sample |
|
📖 Netlify Preview Ready! Preview URL: https://pr-620--sunny-cactus-210e3e.netlify.app (60826d6) 📚 Changed Lecture Pages: imp_sample |
|
📖 Netlify Preview Ready! Preview URL: https://pr-620--sunny-cactus-210e3e.netlify.app (e57deef) 📚 Changed Lecture Pages: imp_sample |
|
📖 Netlify Preview Ready! Preview URL: https://pr-620--sunny-cactus-210e3e.netlify.app (58d1cd2) 📚 Changed Lecture Pages: imp_sample |
|
🤖 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-620--sunny-cactus-210e3e.netlify.app/ currently returns 404. Why previews are down (repo-wide findings)1. This branch is stale — 168 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 Recommended first step for this PRUpdate this branch to This PR touches: |
|
@HumphreyYang thanks for this — the JAX conversion is real work and mostly reads well. I ran the lecture end to end on a GPU box (RTX 4080, JAX 0.10) and it completes in 12 seconds, so the speedup is there. A few things need attention before this can go in, one of them a genuine bug. What's good
Blocker: the final histogram plots the wrong seriesIn the last code cell (the results = simulate_multiple_T(...) # returns (μ_L_p_all, μ_L_q_all), each of shape (n_T, N_simu)
for i, t in enumerate(T_list):
μ_L_p, μ_L_q = results[i] # indexes the tuple, then unpacks along the T axis
Both legends are therefore wrong, and neither panel shows the comparison the surrounding text describes. It raises no error only because μ_L_p_all_h2, μ_L_q_all_h2 = all_results_h2
...
μ_L_p = μ_L_p_all_h2[i]
μ_L_q = μ_L_q_all_h2[i]The Undocumented reduction in sample size
I checked that the narrative still holds — at Could you confirm whether the reduction was deliberate? Either restoring Style-guide items
Merge stateThe branch is currently Happy to help with any of this if useful, but it's your PR and you know the intent behind the sample-size change better than I do — over to you. |
|
@HumphreyYang Claude authored the above, as a list of suggestions to get this merged. Please let me know if you don't have time and I'll handle it. |
|
Thanks @jstac, I am working on this now. |
📖 Netlify Preview Ready!Preview URL: https://pr-620--sunny-cactus-210e3e.netlify.app Commit: 📚 Changed LecturesBuild Info
|
|
Thanks for working on this @HumphreyYang ! Ready for review? |
Yes! Please let me know how to improve it further. |
The likelihood ratio plot was drawn on a linear axis, where the spike at the left-hand edge flattens the rest of the curve onto zero -- so the figure appeared to show the ratio going to zero, contradicting the text directly beneath it. Switch to a log vertical axis, and reference both figures with numref. Fix a NaN: jax.random.beta(key, 0.5, 0.5) returns exactly 1.0 about once per 2e8 draws, and the T=20 importance sampling arm draws exactly that many. At the boundary the log-space beta_pdf evaluates 0 * -inf, so one of the 1000 simulated means came back NaN and was silently dropped by nanmean. Clip draws off the boundary; the reductions become mean/var. Compute the Monte Carlo arm once and reuse it, rather than recomputing it for each importance distribution. It does not depend on the importance distribution, and recomputing it reported different values for the same quantity in different figures. Report a median alongside the mean in the histogram panels. The Monte Carlo estimator is unbiased in population, so its sample mean at T=20 is a very noisy statistic (variance > 100; the reported value ranges from 0.37 to 12.99 across seeds) and does not fall monotonically in T. The median does, and it is what the histograms actually show. Adjust the surrounding text to describe the collapsing distribution rather than a growing bias in the mean. Style-guide items: add the pip install cell and GPU admonition that the other JAX lectures carry; rename figures fig_x -> fig-x; drop figsize (14, 10) on the 1x2 grids; use numpy rather than jax.numpy for plotting; factor the three near-identical histogram blocks into one function; label the reported dispersion as a variance rather than as sigma-hat. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@HumphreyYang thanks — everything from the last round is in, and the lecture reads well. Rather than send you another list I've pushed a commit to this branch ( Runs clean end to end in 34s on an RTX 4080, all seven figures render, and I re-checked every numeric claim in the prose against the new output. What's in the commit, briefly
The one that needs your judgement: mean vs medianThis is the only change that touches the lecture's argument rather than its code, so I want to lay out the reasoning in full. How it surfaced. Computing the Monte Carlo arm once reshuffled the RNG keys. With the new keys the reported MC means came out which is not monotone, and sits directly beneath "Evidently, the bias increases with increases in Why this isn't just an unlucky seed to be swapped out. The Monte Carlo estimator here is unbiased in population — What actually degrades as T grows is the shape of the sampling distribution: almost all the mass collapses toward zero while the expectation is sustained by rare, very large outliers. That is exactly what the red histograms show, and it is the real reason importance sampling is needed. The consequence for the reported number. Because the mean rides on those rare outliers, μ̂ is itself a very noisy statistic at large T — the variance across the 1000 estimates is >100 at T=20. Measuring the reported mean across 15 different seeds (same
So at T=20 the number printed on the figure can be anywhere from 0.37 to 13 depending on the seed. The sequence in the current version of the PR — 1.0, 0.989, 0.899, 0.408 — is monotone, but that is a property of that particular seed rather than of the estimator. The median, by contrast, falls steadily and is stable across seeds. What I did. Each panel now reports a median alongside the mean and variance, and the two sentences after the figure now describe the collapsing distribution rather than a growing bias in the mean:
What I deliberately didn't do. The quick fix would have been to hunt for a seed that makes the means look monotone again. That would be cherry-picking a seed to support a claim the statistic doesn't robustly support, so I left it alone. I recognise this is a change to your exposition and not merely to the code, and you may well prefer a different framing — the underlying point is yours and you may want to make it differently. Reverting to the previous wording is a small edit if you'd rather; the figure change and the prose change are independent of everything else in the commit. Claude wrote this comment and the commit it describes; I've reviewed both. — jstac |
|
@HumphreyYang above is from Claude, i'll review now, you can ignore. |
Mathematical corrections, all pre-existing: The Monte Carlo sentence described the wrong estimator. It named the target as E[L(omega^t)] but then described drawing single omega's and averaging ell(omega), which estimates the t=1 object rather than the mean of the product. It now describes drawing sequences, forming the product along each, and averaging the products -- which is what the code has always done. The central importance sampling display equated estimators where only expectations are equal: E-hat^p[.] = E-hat^q[. p/q] is false, since those are different random variables. Split into a population identity (no hats) and the estimator it suggests (one hat, on the sample average), matching how the single-draw case is already handled earlier in the lecture. The lecture attributed the failure of Monte Carlo to skewness and undersampling. The sharp statement is that the estimator has infinite variance: E^g[ell^2] = int f^2/g diverges, because f is uniform while g vanishes like omega^2 at the origin, so the integrand behaves like omega^-2. Hence no CLT applies. Added the derivation, and used the same criterion -- finite variance iff int f^2/h converges -- to make the lecture's hunches about h1, h2 and h3 precise. For h3 the integral diverges at both endpoints, so it fails for exactly the reason plain Monte Carlo does. Correspondingly weakened a claim added in the previous commit, which said the variance at T=20 "exceeds 100" when it is in fact infinite. Also: restored a dropped expectation operator; settled the importance distribution superscript on q, which had appeared as both h and q within three lines; removed the index collision where t was both the product index and the path length; replaced undefined E_0 with E; w -> omega in three displays; and fixed "suppose were", "arbitraily", "at higher values of distribution", a stray +++ cell break and a missing period. Code simplification: Dropped the hand-rolled beta_pdf for jax.scipy.stats.beta.pdf. These agree to 2e-15 across every parameter pair the lecture uses, and the library version is the same log-space formula written with xlogy/xlog1py -- the primitives that define 0*log(0) = 0, and so precisely what the hand-rolled version lacked at the boundary. Removed jax.jit from f, g, l and estimate_single_path, none of which are called from Python. Per the style guide, decorate the outermost function rather than every helper. Verified that this changes neither the results nor the runtime, since JAX inlines a nested jit at trace time. Verified end to end on a GPU: 34s, 0 NaN and 0 inf across every simulated array, and results unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
A second commit on this branch ( Two things the lecture was saying that it didn't meanThe Monte Carlo sentence described the wrong estimator. It named the target as The code has always done the right thing: The central importance sampling display equated estimators. It asserted The middle equality is false as written — The variance is infinite, not merely largeThe lecture explains the failure of Monte Carlo as skewness plus undersampling. That's right in spirit but understates it. The second moment of a single likelihood ratio is and So The same criterion sharpens the
The lecture's hunch that (One small correction there: "$h_3$ has zero mass at values very close to 0" became "almost no mass". Smaller notational fixesA dropped expectation operator on the right-hand side of one display; the importance distribution superscript appearing as both Code
Verified end to end on a GPU: 34s, 0 NaN and 0 inf across every simulated array, results unchanged. One deliberate non-change: the code still uses Claude wrote this comment and the commit it describes; I've reviewed both. — jstac |
|
@thomassargent30 @HumphreyYang Misc. improvements to the JAX code. Tighted and improved the discussion of importance sampling under alternative h_i. Merging. |
✅ Translation sync completed (zh-cn)Target repo: QuantEcon/lecture-python.zh-cn
|
This is one of the lectures that have the longest runtime.
I made a few big changes that makes the code 4 x faster than Numba optimized version.
Updating in progress...