Skip to content

[multi_hyper] Conversion to JAX and stylesheet check - #657

Merged
jstac merged 3 commits into
mainfrom
multi_hyper_review
Aug 5, 2026
Merged

[multi_hyper] Conversion to JAX and stylesheet check#657
jstac merged 3 commits into
mainfrom
multi_hyper_review

Conversation

@HumphreyYang

Copy link
Copy Markdown
Member

This PR converts the simulation code to JAX and perform stylesheet check!

@github-actions

Copy link
Copy Markdown

📖 Netlify Preview Ready!

Preview URL: https://pr-657--sunny-cactus-210e3e.netlify.app (f4a251b)

📚 Changed Lecture Pages: multi_hyper

@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-657--sunny-cactus-210e3e.netlify.app/ currently returns 404.

Why previews are down (repo-wide findings)

1. This branch is stale — 166 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).

Recommended first step for this PR

Update this branch to main (merge or rebase — pulls in #939 plus ~166 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-657--sunny-cactus-210e3e.netlify.app/multi_hyper.html

This PR touches: multi_hyper.md. Last CI build: success@2025-10-26. Branch: 166 commits behind main as of 2026-07-08.

Per the review on #657, JAX is not justified in this lecture. The
sampling is a single call to np.random.Generator.multivariate_hypergeometric,
a library primitive with no JAX equivalent, so the conversion hand-rolled
it as a lax.scan over individual draws — the "rewriting a library call by
hand" anti-pattern in the style guide. Measured on CPU, which the branch
forced via jax_platform_name, that sampler is slower than NumPy at every
size tested: 87x at 10k, 4.3x at 1M, and 2.7x at the 10M the lecture
actually uses.

Reimplementing the PMF with jax.scipy.special.gammaln also ran in float32,
returning 0.079575970768929 for the first example where the prose two
lines above states the exact value 0.079575596816976.

The branch also added `!pip install jax`, which the style guide forbids
because it can install jax[cpu] rather than the configuration we build
with.

Kept from the branch, on top of main's NumPy implementation:

- vectorize the covariance matrix as
  Σ = n(N-n)/(N-1) · (diag(p) - outer(p, p)), replacing the nested loop
  and its trailing special case for the last diagonal entry; checked
  against the old code, max absolute difference 2.2e-16
- section headings to sentence case, the lecture title keeping title case
- `{code-cell} python3` and `{code-cell} ipython` -> `ipython3`

Co-Authored-By: HumphreyYang <humphreyyang30@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jstac
jstac marked this pull request as ready for review August 5, 2026 19:53
@jstac

jstac commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @HumphreyYang. I've reworked this branch rather than let it sit — here's the reasoning.

Dropping the JAX conversion. The sampling in this lecture is a single call to np.random.Generator.multivariate_hypergeometric, a library primitive that JAX has no equivalent for. Converting therefore meant hand-rolling the sampler as a lax.scan over individual draws, vmap'd across the sample — which is the rewriting a library call by hand anti-pattern in the style guide.

The hand-rolled version is correct — sample means match theory and every row sums to $n$ — but it's slower than NumPy at every size, measured on CPU, which the branch forced via jax_platform_name:

size NumPy this branch
10,000 0.004s 0.358s 87× slower
100,000 0.012s 0.481s 40× slower
1,000,000 0.153s 0.656s 4.3× slower
10,000,000 (what the lecture uses) 1.35s 3.69s 2.7× slower

There was also a subtler consequence. Reimplementing the PMF in log space with jax.scipy.special.gammaln runs in float32, so the first example returned

jax gammaln : 0.079575970768929

where the prose two lines above states the exact value 0.079575596816976. The reader would see the two disagree at the sixth significant figure.

Separately, !pip install jax at the top is something the style guide rules out — it can install jax[cpu] rather than the GPU configuration we build with, which comes from Docker and Actions.

What I kept. Your vectorization of the covariance matrix is a real improvement and doesn't need JAX at all, so it's now applied on top of main's NumPy implementation:

p = K_arr / N
Σ = n * (N - n) / (N - 1) * (np.diag(p) - np.outer(p, p))

That replaces the nested loop and its easily-missed trailing Σ[-1, -1] special case. I checked it against the old code — maximum absolute difference 2.2e-16.

Also kept: the section headings in sentence case, and {code-cell} python3 / {code-cell} ipython brought to ipython3 throughout.

I left the emphasis as it is on main. Most of the bolded items here — "balls", "color", "color blind", "binomial coefficients" — are terms being defined, and the style guide reserves bold for definitions with italic for emphasis, so the existing markup already matches.

I also merged current main (the branch was 220 commits behind, which was why the preview was 404ing) and marked the PR ready. It's now +31/−37 against main.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

📖 Netlify Preview Ready!

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

Commit: 67560aa

📚 Changed Lectures


Build Info

@jstac

jstac commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @HumphreyYang for working on this. Comments above are by Claude. Modifications are by Claude after reading the latest style guide. I'm doing whatever it says because I'm scared of making it angry. I'll merge when green.

@jstac
jstac merged commit a65c025 into main Aug 5, 2026
2 checks passed
@jstac
jstac deleted the multi_hyper_review branch August 5, 2026 20:14
@mmcky

mmcky commented Aug 5, 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#241
Files synced (1):

  • lectures/multi_hyper.md

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants