[mccall_q] Fix typos, heading case and figure legend by bishmaybarik · Pull Request #624 · QuantEcon/lecture-python.myst

bishmaybarik · 2025-09-25T03:03:15Z

This PR is created to convert the mccall_q.md lecture codes into JAX equivalents.

The JAX equivalent of all codes should be written
Fixing the Headings and Sub-Headings to satisfy Quantecon style guidelines
Check for grammatical errors and also see if things are easy to follow through
Adjust the figure legends, as their current position above the curves makes the graphs difficult to interpret.
Rectify the codes with @HumphreyYang 's suggestions
Comment out the codes neatly

github-actions · 2025-09-25T05:54:49Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-09-26T04:59:02Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

earlier the legend overlapped on the curves, making it difficult to interpret the graph. this version fixes it

github-actions · 2025-09-26T07:32:51Z

📖 Netlify Preview Ready!

Preview URL: https://pr-624--sunny-cactus-210e3e.netlify.app (211832b)

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-09-26T07:48:16Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-09-26T07:58:31Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

Copilot

Pull Request Overview

This PR converts the McCall job search model lecture from NumPy/Numba implementation to JAX, updating the code to use JAX's functional programming paradigm and JIT compilation. The conversion includes modernizing the code structure, fixing grammatical errors, and improving figure presentation.

Key changes include:

Complete conversion from NumPy/Numba to JAX with functional programming approach
Replacement of class-based structure with NamedTuple and standalone functions
Implementation of JAX's immutable array operations and control flow primitives

Copilot · 2025-09-29T12:51:31Z

+        qtable, s, accept_count, t, key = state
+        # for first interaction, just continue since error is large
+        # for subsequent interactions, compute actual error
+        error = jnp.where(t==0, δ + 1, jnp.max(jnp.abs(qtable - state[0])))


The error calculation references state[0] but state is a tuple where the first element is qtable. This creates a circular reference where the error is computed as the difference between qtable and itself, which will always be zero after the first iteration. This should reference a previous qtable value stored separately.

Copilot · 2025-09-29T12:51:31Z

-
    for n in range(N):
-        if n%(N/10)==0:
+        if n % (N // 10) == 0:


Integer division N // 10 can result in zero when N < 10, causing a division by zero error in the modulo operation. This should use max(1, N // 10) to ensure the divisor is never zero.

Suggested change

if n % (N // 10) == 0:

if n % max(1, N // 10) == 0:

Copilot · 2025-09-29T12:51:31Z

+      max_epochs = int(jnp.max(epochs_to_plot))  # Convert to Python int
+      # iterate on epoch numbers
+      for n in range(max_epochs + 1):
+          if n%(max_epochs/10)==0:


Division by zero error can occur when max_epochs is 0. The condition n%(max_epochs/10)==0 will fail. This should use max_epochs > 0 and n%(max_epochs/10)==0 or n%(max(1, max_epochs/10))==0.

Suggested change

if n%(max_epochs/10)==0:

if n % max(1, max_epochs // 10) == 0:

HumphreyYang

Many thanks @bishmaybarik! Nice changes -- they are really good improvements. Here are some minor comments.

bishmaybarik · 2025-09-30T10:13:42Z

hi @HumphreyYang , these are extremely useful suggestions -- thanks a lot! I'll make all the necessary changes and push the updated version once ready.

vectorized computation instead of repeatedly resetting arrays with for loops

github-actions · 2025-10-01T06:48:20Z

📖 Netlify Preview Ready!

Preview URL: https://pr-624--sunny-cactus-210e3e.netlify.app (9143f6b)

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-10-01T07:07:21Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-10-06T05:49:25Z

📖 Netlify Preview Ready!

Preview URL: https://pr-624--sunny-cactus-210e3e.netlify.app (06e07c0)

📚 Changed Lecture Pages: mccall_q

github-actions · 2025-10-06T05:58:49Z

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: mccall_q

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Copilot · 2025-10-06T08:37:02Z

+        qtable, s, accept_count, t, key = state
+        # for first interaction, just continue since error is large
+        # for subsequent interactions, compute actual error
+        error = jnp.where(t==0, δ + 1, jnp.max(jnp.abs(qtable - state[0])))


The error calculation references state[0] which should be the previous qtable. This creates a circular reference since state is the current state tuple. Consider storing the previous qtable separately or using a different approach to calculate the error.

bishmaybarik · 2025-10-13T02:27:11Z

hi @HumphreyYang and @mmcky , may I know if you have any thoughts on these updates? It would be great to hear from you!

jstac · 2026-07-08T06:03:32Z

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

Why previews are down (repo-wide findings)

1. This branch is stale — 175 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 ~175 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-624--sunny-cactus-210e3e.netlify.app/mccall_q.html

This PR touches: mccall_q.md. Last CI build: success@2025-10-06. Branch: 175 commits behind main as of 2026-07-08.

jstac · 2026-07-20T21:34:44Z

From @bishmaybarik :

hi @HumphreyYang and @mmcky , may I know if you have any thoughts on these updates? It would be great to hear from you!

From me: I can handle this if you don't have time, just let me know.

Resolve the mccall_q.md conflict by taking main's version. The JAX conversion is being dropped: mccall_q is inherently sequential Q-learning, which the style guide directs to Numba rather than JAX (see styleguide/jax.md, "Sequential: Numba wins"). Main has also since moved these functions to the np.random.default_rng Generator API in #959. The prose, heading and figure-legend improvements from this branch are reapplied in the next commit.

Reapplies the non-JAX improvements from the original JAX-conversion work on top of main's Numba implementation: - typo and wording fixes: "illegitmate", "previos", "prematurally", "algorthm", "objection to reject" -> "option to reject", "take existing wage if and update" -> "take the existing wage and update", "$Q$ map" -> "$Q$ maps", "vis a vis" -> "versus" - section headings to sentence case, per the style guide (the lecture title keeps title case) - move the crowded legend in plot_epochs below the axes in two columns, where it no longer covers the curves, and format the mean error to two decimals so the labels fit - drop a duplicated VFI computation that ran at the end of the wage distribution plotting cell and again in the cell immediately after - guard the progress-report modulus against small epoch counts Co-Authored-By: bishmaybarik <177107536+bishmaybarik@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jstac · 2026-08-05T04:40:53Z

Thanks @bishmaybarik, and apologies for the slow review — this sat much longer than it should have.

I've taken the branch in a different direction rather than let it go stale, so I want to explain the reasoning.

On the JAX conversion. I don't think mccall_q should be a JAX lecture. Q-learning here is inherently sequential: run_one_epoch walks up to 20,000 steps, each reading and writing a single cell of an 11×2 Q-table based on the step before it. There's nothing to vectorize and nothing to parallelize. That's the case the style guide addresses directly under Sequential: Numba wins:

For inherently sequential work that does not need autodiff, prefer Numba. At best JAX ties on speed — and only after being pinned to the CPU — while the syntax is considerably less clear.

The branch itself makes the point: it sets jax.config.update('jax_platform_name', 'cpu') in the imports cell. This repo's CI runs on a GPU instance with jax[cuda12-local], so the conversion moves the lecture to JAX and then switches off the hardware that would justify it. That's a reliable signal the algorithm isn't a fit.

This is a judgment call about the lecture, not about the work — the conversion itself is competent, and the NamedTuple + factory-function structure is exactly what the guide asks for. It's just applied to a lecture that shouldn't have been on the conversion list.

One thing worth knowing for future conversions. There's a bug in the converted run_one_epoch that's easy to miss:

def cond_fun(state):
    qtable, s, accept_count, t, key = state
    error = jnp.where(t==0, δ + 1, jnp.max(jnp.abs(qtable - state[0])))

state[0] is qtable — unpacked from it on the line above — so error is identically zero for every t > 0 and the loop exits after one iteration. I ran it to confirm: each "epoch" performs exactly one TD update instead of running to convergence. The Numba original compared the pre- and post-update tables (qtable_new - qtable); that comparison got lost in the rewrite. Worth watching for when converting a while loop whose exit test depends on the change in the carry — the carry has to hold both the old and new values, or the test has to move into the body.

What I kept. The branch had real improvements independent of JAX, and those are now applied on top of main:

all the typo and wording fixes — "illegitmate", "previos", "prematurally", "algorthm", "objection to reject" → "option to reject", "$Q$ map" → "$Q$ maps"
section headings to sentence case, per the style guide
the legend fix in plot_epochs — it now sits below the axes in two columns instead of covering the curves. I also formatted the mean error to two decimals, since the full-precision labels were too wide to fit
the duplicated VFI computation at the end of the wage-distribution cell, which ran again in the very next cell

Main had also moved these functions to the np.random.default_rng Generator API in #959, so I merged main in to resolve that. Your commits are still in the branch history.

Retitled accordingly. Thanks again for the work here — the reference material is in the style guide's when to use JAX checklist if you're picking up another conversion.

github-actions · 2026-08-05T04:45:56Z

📖 Netlify Preview Ready!

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

Commit: 46c1f40

📚 Changed Lectures

mccall_q

Build Info

Workflow: Build Project [using jupyter-book]

mmcky · 2026-08-05T04:56:51Z

✅ Translation sync completed (zh-cn)

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

lectures/mccall_q.md

update import block

edd7608

bishmaybarik self-assigned this Sep 25, 2025

bishmaybarik added 2 commits September 25, 2025 09:07

convert mccall model from numba to jax

61004bc

complete jax conversion of q-learning implementation

c8814f3

bishmaybarik added in-work jax-conversion labels Sep 25, 2025

fix headings and subheadings to match style guide

c575e39

bishmaybarik added 3 commits September 26, 2025 12:31

rectify grammatical errors in the lecture

211832b

fix legend and place it below graph

d7b49a6

earlier the legend overlapped on the curves, making it difficult to interpret the graph. this version fixes it

update jupytext version

ef3e791

bishmaybarik marked this pull request as ready for review September 26, 2025 07:30

bishmaybarik requested review from HumphreyYang and mmcky September 26, 2025 07:31

bishmaybarik added review and removed in-work labels Sep 26, 2025

HumphreyYang requested a review from Copilot September 29, 2025 12:50

Copilot AI reviewed Sep 29, 2025

View reviewed changes

HumphreyYang reviewed Sep 30, 2025

View reviewed changes

bishmaybarik added 4 commits October 1, 2025 11:48

use seed equals 0 as default

9143f6b

use vectorized computation

a72d9b3

vectorized computation instead of repeatedly resetting arrays with for loops

replace eps (epsilon) with the unicode character

f1a7dca

remove unnecessary comment

f53c4ef

bishmaybarik added 9 commits October 1, 2025 13:08

shorten code length to have each line under 80 characters

088f91f

follow copilot's suggestion to prevent division by 0

75db66d

shorten lengthy notes

6613e32

replace loop with jax.vmap for vectorized value update

d6034fe

make PRNG key explicit in plot_epochs calls for consistent results

a65f940

prevent division by zero as per copilot

659554f

replace valfunc_VFI with valfunc_vfi throughout the lecture

dbcfd78

fix a typo in the lecture

06e07c0

add minor comments into the code

d5d2565

bishmaybarik requested a review from HumphreyYang October 6, 2025 05:42

bishmaybarik requested a review from Copilot October 6, 2025 08:36

Copilot AI reviewed Oct 6, 2025

View reviewed changes

mmcky mentioned this pull request Aug 3, 2026

TESTING: review + expert-parallel protocol for the benchmark skill (@xuanguang-li) QuantEcon/skills#10

Open

13 tasks

jstac and others added 2 commits August 5, 2026 14:26

jstac changed the title ~~[mccall_q]: JAX conversion~~ [mccall_q] Fix typos, heading case and figure legend Aug 5, 2026

jstac removed the jax-conversion label Aug 5, 2026

jstac merged commit 46cf999 into main Aug 5, 2026
2 checks passed

jstac deleted the jax-mccallq branch August 5, 2026 04:54

mmcky mentioned this pull request Aug 5, 2026

🌐 [translation-sync] [mccall_q] Fix typos, heading case and figure legend QuantEcon/lecture-python.zh-cn#236

Open

This was referenced Aug 7, 2026

TRACKING: work plan from 2026-07-29 — run 2 against meta, and the block-2 judgement calls QuantEcon/skills#25

Open

PLAN: benchmark — from v0 outline to operational /benchmark:review-acceleration QuantEcon/skills#4

Open

	if n%(max_epochs/10)==0:
	if n % max(1, max_epochs // 10) == 0:

Uh oh!

Uh oh!

Conversation

bishmaybarik commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 25, 2025

Uh oh!

github-actions Bot commented Sep 26, 2025

Uh oh!

github-actions Bot commented Sep 26, 2025

Uh oh!

github-actions Bot commented Sep 26, 2025

Uh oh!

github-actions Bot commented Sep 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

Uh oh!

HumphreyYang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bishmaybarik commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 1, 2025

Uh oh!

github-actions Bot commented Oct 1, 2025

Uh oh!

github-actions Bot commented Oct 6, 2025

Uh oh!

github-actions Bot commented Oct 6, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

bishmaybarik commented Oct 13, 2025

Uh oh!

jstac commented Jul 8, 2026

Why previews are down (repo-wide findings)

Recommended first step for this PR

Uh oh!

jstac commented Jul 20, 2026

Uh oh!

jstac commented Aug 5, 2026

Uh oh!

github-actions Bot commented Aug 5, 2026

📖 Netlify Preview Ready!

📚 Changed Lectures

Uh oh!

Uh oh!

mmcky commented Aug 5, 2026

✅ Translation sync completed (zh-cn)

Uh oh!

Reviewers

Assignees

bishmaybarik commented Sep 25, 2025 •

edited

Loading

bishmaybarik commented Sep 30, 2025 •

edited

Loading