Skip to content

[exchangeable] Simplify code and bring lecture into style-guide compliance - #772

Merged
jstac merged 7 commits into
mainfrom
exchangeable_fixes
Aug 3, 2026
Merged

[exchangeable] Simplify code and bring lecture into style-guide compliance#772
jstac merged 7 commits into
mainfrom
exchangeable_fixes

Conversation

@kp992

@kp992 kp992 commented Dec 26, 2025

Copy link
Copy Markdown
Contributor

Update the lecture to use JAX and removes numba related code. Also fixes minor typos and code styling issues.

@github-actions

Copy link
Copy Markdown

📖 Netlify Preview Ready!

Preview URL: https://pr-772--sunny-cactus-210e3e.netlify.app (5098c0e)

📚 Changed Lecture Pages: exchangeable

@kp992
kp992 requested review from HumphreyYang and mmcky December 26, 2025 02:10
@jstac

jstac commented May 30, 2026

Copy link
Copy Markdown
Contributor

@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?

@jstac

jstac commented May 30, 2026

Copy link
Copy Markdown
Contributor

@longye-tian or any other reviewer: Does adding JAX improve this lecture? Please also make this value judgement.

@longye-tian longye-tian left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kp992 @jstac,

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.scan for the time recursion and vmap to 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:

  1. likelihood ratio plot
  2. density / probability-region plot
  3. 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 explicit seed or key argument. That would make the source of randomness clearer.
  • The comment # Generate all random keys upfront appears inside simulate_path, but the keys are actually generated in simulate; 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_rarioexpected_ratio

Best,
Longye

@jstac jstac closed this Jul 7, 2026
@jstac jstac reopened this Jul 7, 2026
@jstac

jstac commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

🤖 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 main. A preview build compiles the whole site from this branch. This branch's lectures/house_auction.md still has unpinned !pip install prettytable, which now breaks on a wcwidth incompatibility. main fixed this on 2026-06-28 by pinning prettytable<3.18 (#939). This alone fails any rebuild of this branch until it's updated to main.

2. The arviz failure was a red herring — do NOT pin arviz or rewrite plotting. A 2026-07-07 rebuild also failed in ar1_bayes/ar1_turningpts with an arviz_plots figsize ValueError. That was a transient bug in an intermediate arviz-plots 1.x release, already fixed in arviz 1.2.0. Verified locally on a clean latest-stack venv: the real az.plot_trace(trace) cell (pymc + numpyro InferenceData) runs green. The lectures use only 1.x-compatible arviz APIs (plot_trace, summary, from_numpyro, compare).

Note on recent timeline activity

This 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 PR

Update this branch to main (merge or rebase — pulls in #939 plus ~93 other commits), then let CI rebuild. On today's latest libraries the site builds clean, so the preview should return. house_auction is the known blocker; updating also picks up other since-merged fixes — rebuild and address any remaining per-lecture failures. Verify with:

curl -sI https://pr-772--sunny-cactus-210e3e.netlify.app/exchangeable.html

This PR touches: exchangeable.md. Last CI build: failure@2026-07-07. Branch: 93 commits behind main as of 2026-07-08.

@kp992

kp992 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @longye-tian. I will review your comment and make the changes.

@kp992
kp992 requested a review from longye-tian July 15, 2026 21:54
@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

📖 Netlify Preview Ready!

Preview URL: https://pr-772--sunny-cactus-210e3e.netlify.app

Commit: 4d918b2

📚 Changed Lectures


Build Info

@kp992
kp992 requested a review from jstac July 31, 2026 23:29
jstac added 2 commits August 3, 2026 07:56
…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.
@jstac jstac changed the title [exchangeable] Update lecture to use JAX [exchangeable] Simplify code and bring lecture into style-guide compliance Aug 3, 2026
@jstac

jstac commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

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.

Summary

I'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 JAX

Longye's instinct about learning_example was right, and the JAX style guide names this case explicitly under when not to use JAX:

Converting only the plots. Figure generation is never the bottleneck.

and

Substituting jnp for np is not converting a lecture to JAX. If the numerical work still runs through SciPy, scipy.stats, or NumPy-based QuantEcon.py classes, then the arrays are merely being shuttled between host and device at every call. The lecture gets JAX's constraints and none of its benefits.

That is exactly the situation in learning_example and expected_ratio: the actual computation is scipy.optimize.root_scalar and scipy.integrate.quad over 100-point plotting grids. Measured locally, results were identical but the jitted version of that section took 0.245s against 0.0004s for plain NumPy — roughly 600× slower, because quad and root_scalar call the jitted function scalar-by-scalar and pay dispatch overhead on every one.

That left only the appendix simulation as a genuine JAX candidate, and that turned out to have a much better answer than either lax.scan or a Numba loop — see below. With no JAX work left, jax.config.update("jax_enable_x64", True) went too. (For the record I did test whether it was needed: with the same draws, float32 and float64 belief paths differ by at most 1.5e-3, which is invisible among 1000 plotted paths.)

What replaced it

1. The simulation is now a cumulative product. Writing Bayes' Law in odds form,

$$\frac{\pi_{t+1}}{1-\pi_{t+1}} = l(w_{t+1}),\frac{\pi_t}{1-\pi_t} \quad\Longrightarrow\quad \frac{\pi_t}{1-\pi_t} = \frac{\pi_{-1}}{1-\pi_{-1}}\prod_{s=0}^{t} l(w_s)$$

so the whole ensemble of belief paths is a np.cumprod of likelihood ratios — no lax.scan, no vmap, no Python loop. simulate is now four lines. Both equations are in the lecture, so the code follows visibly from the mathematics rather than looking like a trick, and it earns the sentence about likelihood ratio processes that was already in the text but previously unexplained. Verified against the sequential recursion: agreement to ~1e-12.

2. learning_example is split, as Longye suggested. There is now a create_model that builds f, g and the two roots of l(w) = 1, plus plot_likelihood_ratio, plot_densities and plot_belief_dynamics — one per panel. The text introduces and discusses each graph in turn instead of asking the reader to parse a page-long helper before seeing the three ideas it produces.

3. Random number generation now uses np.random.default_rng with an explicit generator passed in, per the style guide.

Style-guide fixes

Some of these predate this PR, but they were worth fixing while the file was open:

  • Four instances of {doc}`this lecture <...>` replaced with the auto-title form ({doc}`odu` etc.) — see Using lecture titles in {doc} references. This file had more of these than any other lecture in the repo.
  • The hard-coded python-advanced.quantecon.org URL replaced with the intersphinx reference {doc}`advanced:additive_functionals` .
  • Seven section headings lower-cased — full capitalization is for the lecture title only.
  • kp992's typo fixes retained, plus two more found on main: "If a sequence is random variables is IID" and "something to learn about about the future".

Verification

All twelve figures were rendered and compared against main. The three original panels reproduce exactly — same curves, same shaded regions, same printed areas — so the hard-coded numbers in the prose (0.524, 0.476, 0.816) still hold. The simulation figures match in shape and magnitude, differing only through the new generator, as expected.

Runtime for the whole lecture:

version time
this branch 2.6s
main (Numba) 3.4s
previous state of this PR (JAX) 7.8s

One footnote for anyone reading the diff: I first wrote the Beta density as scipy.stats.beta.pdf, which reads more cleanly, and it took the lecture from 3.4s to 47s. beta.pdf costs ~50µs per call against ~0.3µs for the explicit math.gamma formula, and quad calls it scalar-by-scalar. Worth remembering generally — scipy.stats .pdf methods are a poor fit inside quadrature.

@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.

@jstac

jstac commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@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).

@jstac
jstac merged commit 7e4c793 into main Aug 3, 2026
1 check passed
@jstac
jstac deleted the exchangeable_fixes branch August 3, 2026 02:01
@mmcky

mmcky commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

✅ Translation sync completed (zh-cn)

Target repo: QuantEcon/lecture-python.zh-cn
Translation PR: QuantEcon/lecture-python.zh-cn#233
Files synced (1):

  • lectures/exchangeable.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants