[ifp_advanced] Align K with the (c, a) convention used across the EGM lectures - #1016
Merged
Conversation
… lectures `K` in this lecture was written as `K(a_in, c_in, ifp) -> (a_out, c_out)`, while `solve_model` — copied verbatim from `ifp_egm.md` — calls it as `c_out, a_out = K(c_in, a_in, ifp)`. That double swap on both the arguments and the unpacking left the code numerically correct but made every name in `solve_model` mean the opposite of what it says: its first parameter was really the asset grid and its first return value the asset grid too, despite the signature and docstring claiming consumption. `ifp_advanced.md` was the only lecture in the family with this inversion. Both `ifp_egm.md` and `ifp_egm_transient_shocks.md` define `K(c_in, a_in, ifp) -> (c_out, a_out)`, and their `solve_model` is otherwise identical to this one. So the fix is to `K`, not to `solve_model`: swap its first two parameters and its return order, then update the five call sites to the `(c, a)` ordering the rest of the family — and this lecture's own `simulate_household` and `compute_asset_stationary` — already use. Verified by executing the lecture on jax 0.11.0 and comparing against main: Gini 0.918535 -> 0.918532 (0.9185 as displayed, unchanged) top 1% share 0.8982 -> 0.8982 median / p99 identical policy arrays agree to 2.3e-05, inside the solver's own 1e-05 tol The residual difference is a side benefit rather than drift. `solve_model` measures convergence as `max|c_out - c_in|`, which previously landed on the *asset grid* and so computed `(s + c_out) - (s + c_in)` in float32 — a cancellation whose granularity is one ulp of the grid maximum, exactly 7.629395e-06 at s = 100, against a tolerance of 1e-05. The criterion was dominated by round-off. Measuring the consumption residual directly is well conditioned and converges in 158 iterations rather than 160. Supersedes #762, which changed the five call sites to this ordering without touching `K`. That combination runs without raising but reverses the x and y arguments of the `jnp.interp` in `simulate_household`; executed, it returns a Gini of -0.9995 and a minimum wealth of -4.1e+07. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing readability defect in ifp_advanced.md by aligning the Coleman–Reffett operator K with the (c, a) argument/return convention used across the EGM lecture family, so variable names in solve_model match what they actually represent.
Changes:
- Reordered
K’s first two parameters to(c_in, a_in, ifp)and changed its return order to(c_out, a_out). - Updated the five
solve_modelcall sites to pass(c_init, a_init)and unpack(c_out, a_out)consistently (including the timed.block_until_ready()target).
Contributor
Author
|
@jstac a third set of eyes on this would be great if you have time. I am pretty sure this is correct now. |
Contributor
|
Looks good @mmcky , many thanks! pls merge when ready. |
📖 Netlify Preview Ready!Preview URL: https://pr-1016--sunny-cactus-210e3e.netlify.app Commit: 📚 Changed LecturesBuild Info
|
Contributor
Author
✅ Translation sync completed (zh-cn)Target repo: QuantEcon/lecture-python.zh-cn
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #762, which is now closed. That PR read the symptom correctly but fixed the wrong end of it; this one fixes the actual defect and is verified by execution.
What is wrong
Kinifp_advanced.mdis written asK(a_in, c_in, ifp) -> (a_out, c_out), butsolve_model— copied verbatim fromifp_egm.md— calls it asc_out, a_out = K(c_in, a_in, ifp). That is a swap on both the arguments and the unpacking, so the two cancel and the code is numerically correct. What it is not is readable: every name insolve_modelends up meaning the opposite of what it says. Its first parameter is documented asc_init: Initial guess of σbut is really the asset grid, and its first return value is the asset grid too. A student comparing the signature at line 441 against the call site at line 490 sees a flat contradiction, in a lecture whose entire point is that the source is read.ifp_advanced.mdis the only lecture in the family with this inversion. Bothifp_egm.mdandifp_egm_transient_shocks.mddefineK(c_in, a_in, ifp) -> (c_out, a_out), and theirsolve_modelis otherwise byte-identical to this one. This lecture's ownsimulate_householdandcompute_asset_stationaryalso already take(c_vec, a_vec)in that order.The fix
Swap
K's first two parameters and its return order so it matches its siblings, leavingsolve_modeluntouched, then update the five call sites to the(c, a)ordering everything else already uses. Ten lines, no change to any computation.Verification
Executed the lecture end to end on jax 0.11.0 and diffed against
main:Both display as
0.9185in the lecture output, so nothing visible moves.The 2.3e-05 residual is a side benefit rather than drift, and it is worth recording.
solve_modelmeasures convergence asmax|c_out - c_in|. Onmainthat expression landed on the asset grid, so it evaluated(s + c_out) - (s + c_in)in float32 — a cancellation whose granularity is one ulp of the grid maximum, which ats = 100is exactly7.629395e-06against a tolerance of1e-05. The stopping rule was measuring round-off, andmain's reported final error is precisely that ulp. Measuring the consumption residual directly is well conditioned; it converges in 158 iterations instead of 160, and the two solutions differ well inside the solver's own tolerance.Why #762 could not be merged
It applied the same five call-site edits without touching
K, which reverses the x and y arguments of thejnp.interpinsimulate_household. It raises nothing — consumption is monotone, so the interpolation is perfectly happy — it just returns nonsense. Executed, it gives a Gini of -0.9995 (outside the possible range of the statistic entirely), a mean wealth of -3389 and a minimum of -4.1e+07. Worth flagging as a reminder that a silently-passing notebook is not evidence of a correct one here.