QuantEcon / QuantEcon/lecture-python.myst

rs_inventory_q / inventory_q: misleading 'Converged' print + snapshot off-by-one in Q-learning loop

Open
#882 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TeX
Stars
123
Forks
57
Avg merge
3d 10h
Merged PRs (30d)
11

Description

While reviewing lectures/rs_inventory_q.md as part of mirroring it into QuantEcon/lecture-dp, Copilot flagged three things — all real, none blocking, but worth a small follow-up here.

Issue 1 — `solve_rs_inventory_model` prints "Converged" unconditionally

`rs_inventory_q.md` (and the same pattern in `inventory_q.md` line 331):

def solve_rs_inventory_model(v_init, model, max_iter=10_000, tol=1e-6):
    v = v_init.copy()
    i, error = 0, tol + 1
    while i < max_iter and error > tol:
        new_v = T_rs(v, model)
        error = np.max(np.abs(new_v - v))
        i += 1
        v = new_v
    print(f"Converged in {i} iterations with error {error:.2e}")
    ...

If the loop exits because `i == max_iter` (before `error <= tol`), the print still claims convergence. With the default 10k cap that won't bite at the configured parameters, but it's misleading for anyone who tweaks settings.

Suggestion: only print "Converged" when `error <= tol`, otherwise warn (or raise). Same fix applies in `inventory_q.md`.

Issue 2 — snapshot recorded at the start of the loop iteration

`rs_inventory_q.md` (`q_learning_rs_kernel`):

for t in range(n_steps):
    # Record policy snapshot if needed
    if snap_idx < n_snaps and t == snapshot_steps[snap_idx]:
        snapshots[snap_idx] = greedy_policy_from_q_rs(q, K)
        snap_idx += 1
    ...
    q[x, a] = (1 - α) * q[x, a] + α * target
    ...

The snapshot at index `t` captures `q` before the update for step `t`. So a "snapshot at step n" actually reflects `q` after `n` completed updates only if `t = n` is visited after the n-th update — which requires running `n + 1` iterations.

That's exactly what the caller does, but it's load-bearing for the narrative…

Issue 3 — narrative vs. code mismatch in "Running Q-learning"

`rs_inventory_q.md` line 658:

We run $n$ = 5 million steps and take policy snapshots at steps 10,000, 1,000,000, and $n$.

n = 5_000_000
snap_steps = np.array([10_000, 1_000_000, n], dtype=np.int64)
q_table, snapshots = q_learning_rs(model, n_steps=n+1, snapshot_steps=snap_steps)

`n_steps=n+1` is the workaround for Issue 2 — without it, the snapshot at step `n` would not capture the final update. Two ways to make the narrative consistent:

  1. Move the snapshot recording to after the Q update (or after the loop), then call with `n_steps=n`.
  2. Or keep the snapshot at the start of the iteration but adjust indices so the caller passes `n_steps=n` and `snapshot_steps=[..., n-1]` etc.

Option 1 is cleaner and would let the prose stay literal ("run `n` steps").


Happy to send a small PR if useful — none of these are blocking and `lecture-dp` is mirroring as-is so this stays a single-source fix.

🤖 Found via Copilot review on QuantEcon/lecture-dp#20

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with solve_rs_inventory_model and q_learning_rs_kernel in lectures/rs_inventory_q.md, then compare the mirrored solve function in lectures/inventory_q.md and the “Running Q-learning” section. Check the loop termination message, snapshot timing, and n versus n+1 call semantics. Done means both lecture files report non-convergence accurately and the prose, snapshot indices, and executed step count agree.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
documentation
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.