Skip to content

FIX: lecture cleanup batch from the 2026-08 tracker audit - #595

Open
mmcky wants to merge 4 commits into
mainfrom
lecture-cleanup-2026-08
Open

FIX: lecture cleanup batch from the 2026-08 tracker audit#595
mmcky wants to merge 4 commits into
mainfrom
lecture-cleanup-2026-08

Conversation

@mmcky

@mmcky mmcky commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

One batch of small, independently verified fixes from the 2026-08 tracker audit. They are grouped into a single PR because every item touches lecture source, so batching means the .fa/.fr/.zh-cn editions pick everything up in one translation sync instead of five.

Closes #589, closes #580, closes #552, closes #529, closes #594.

Changes

Issue File(s) Change
#589 polars.md Exercise 1 solution now guards first()/last() with drop_nulls(), matching Exercise 2 — without it, tickers on different trading calendars produce null pct_change values that flow silently into the bar chart
#580 pandas_panel.md The three github.com/QuantEcon/data-lectures/raw/main/... URLs now point directly at raw.githubusercontent.com, avoiding the 302 redirect
#552 (1) python_by_example.md The white-noise snippet now includes the rng = np.random.default_rng() line and the prose counts four lines, not three (also fixes a "genererate" typo)
#552 (2) functions.md The three generate_data cells define rng locally, so running a cell in isolation no longer raises NameError
#552 (3) scipy.md The linregress example defines rng in its own cell (matching the pattern already used at the Monte Carlo example later in the lecture)
#552 (4) numpy.md DiscreteRV.__init__ docstring now documents the seed parameter and the non-reproducibility of the seed=None default
#552 (5) about_py.md "Pytorch" → "PyTorch" (2 sites)
#552 (6) polars.md, pandas.md The eight legacy np.random.* call sites in polars.md migrated to the Generator API; the mirrored Series cell in pandas.md (np.random.randn(4), the one site the audit had not flagged) is migrated too so the twin lectures stay consistent
#529 autodiff.md The simulated-data cell splits the PRNG key once into three single-use keys instead of consuming key with uniform and then re-splitting it (correlated-draws risk); uses the fix suggested in the issue, with ϵ_key named to match the lecture's unicode convention
#594 numpy.md Exercise text now says qe.Timer() context manager instead of the retired tic/toc wording, matching the solution code

Notes for review

Random streams change in the edited cells: the polars/pandas Series and DataFrame demo cells now draw from a fresh Generator, and the polars benchmark block seeds via np.random.default_rng(42) instead of np.random.seed(42). Displayed numbers in the published output will differ from the current build, but nothing pedagogical depends on the specific draws. This is the same trade accepted throughout the series-wide Generator migration (#538#549).

The rng-scoping fixes follow the convention already used by the migrated lectures (numba.md, numpy.md define rng in each cell/section that uses it) rather than introducing a shared top-of-lecture definition.

🤖 Generated with Claude Code

- polars: guard Exercise 1 first/last with drop_nulls (#589)
- polars, pandas: migrate the remaining legacy np.random.* call sites
  to the Generator API (#552)
- pandas_panel: use direct raw.githubusercontent.com data URLs (#580)
- python_by_example: include the rng line in the white-noise snippet
  and correct the line count (#552)
- functions, scipy: define rng in the cells that use it (#552)
- numpy: document DiscreteRV's seed parameter (#552) and reference
  qe.Timer() instead of the retired tic/toc wording (#594)
- about_py: standardize on PyTorch (#552)
- autodiff: split the PRNG key once into three single-use keys (#529)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 3, 2026 23:05

Copilot AI 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.

Pull request overview

This PR applies a batch of small, audit-driven fixes across multiple lecture sources so that all translated editions pick them up in the next sync.

Changes:

  • Fix RNG scoping / modernize random draws in several lectures (NumPy Generator API usage and per-cell rng definitions where needed).
  • Improve correctness/robustness in examples (Polars drop_nulls() guard for first()/last(), JAX single-use PRNG key splitting).
  • Clean up lecture text and links (raw GitHub CSV URLs, docstring/text consistency, PyTorch capitalization).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lectures/scipy.md Defines rng locally in the linregress example cell to avoid hidden dependencies.
lectures/python_by_example.md Aligns prose and snippet for white-noise example (adds rng line; fixes typo/count).
lectures/polars.md Migrates random draws to Generator API; seeds benchmark via default_rng(42); guards first/last with drop_nulls().
lectures/pandas.md Keeps the mirrored Series example consistent with polars.md by using Generator API.
lectures/pandas_panel.md Switches dataset URLs to direct raw.githubusercontent.com links (no redirect).
lectures/numpy.md Documents seed in DiscreteRV.__init__ docstring; updates exercise text to reference qe.Timer() instead of tic/toc.
lectures/functions.md Defines rng in each relevant code cell so running cells in isolation won’t raise NameError.
lectures/autodiff.md Fixes JAX key reuse by splitting once into single-use subkeys for independent draws.
lectures/about_py.md Standardizes “PyTorch” capitalization in prose and links.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

@github-actions
github-actions Bot temporarily deployed to pull request August 3, 2026 23:13 Inactive
@mmcky

mmcky commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@Chihiro2000GitHub would you have time to cast an eye over these changes. Many of them are rng updates.

@mmcky
mmcky requested a review from Chihiro2000GitHub August 3, 2026 23:15
@Chihiro2000GitHub

Copy link
Copy Markdown
Collaborator

Thanks @mmcky!

I've had a quick look through the changes.

I'll do a proper review starting this weekend and get back to you with any comments 😊

The three added `rng = np.random.default_rng()` lines re-bound a generator
that is already created at the top of the "Random Draws" section and carried
forward through notebook state, so they changed nothing at execution time
while adding boilerplate to cells whose subject is function structure.

Reverts functions.md to match main.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky

mmcky commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed the lectures/functions.md change and reverted it in b13f58bfunctions.md now matches main and drops out of this PR.

To answer the question that prompted the look: the rng object was used in all three cells the change touched — rng.standard_normal() in funcloopprog, rng.uniform(0, 1) / rng.standard_normal() in funcloopprog2, and rng.uniform passed as the callable at the call site in test_program_6 (that last one is easy to misread in the diff view, since the function body only calls generator_type()).

The problem was that the three added rng = np.random.default_rng() lines were redundant rather than unused. rng is already bound in the first cell of the "Random Draws" section and notebook state carries it forward, so re-creating it changed nothing at execution time — each default_rng() is unseeded either way, so there was no reproducibility argument for it either.

The change was also inconsistent about it: the draw_new cell in the exercise solutions still calls rng.uniform() with no local constructor, relying on the earlier binding. So the "make each cell self-contained" rationale wasn't applied uniformly.

Given the lecture establishes rng once at the top of the section, and these particular cells exist to teach function structure rather than RNG setup, dropping the boilerplate is the cleaner outcome. If we do want self-contained cells as a repo-wide convention, that is worth doing deliberately across every cell that touches rng — not partially in one lecture.

Comment thread lectures/numpy.md
to 1
to 1.

The argument seed sets the seed for the underlying random number

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jstac what is your view on docstrings in lectures. Should they fully document a function interface (like in a library) or be a concise one sentence description?

@github-actions
github-actions Bot temporarily deployed to pull request August 5, 2026 02:53 Inactive
scipy.md: drop the redundant rng re-creation before the linregress example,
which re-bound a generator already created earlier in the lecture. Reverts
scipy.md to match main.

polars.md: rename the seeded benchmark generator to bench_rng so it no longer
shadows the unseeded rng created in the Series section with one carrying
different semantics.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky

mmcky commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Followed up on the remaining rng usage across the PR (0b077ba). Two tidy-ups:

scipy.md — dropped the added rng = np.random.default_rng() before the linregress example. Same situation as functions.md: rng is already bound earlier in the lecture and carried forward through notebook state, so the line re-created a generator that was already in scope. scipy.md now matches main and drops out of this PR.

polars.md — renamed the seeded benchmark generator to bench_rng. Replacing np.random.seed(42) with rng = np.random.default_rng(42) was the right modern equivalent, but it silently rebound the unseeded rng from the Series section to a seeded one with different semantics. Nothing downstream depended on it, so this was cosmetic rather than a bug, but the distinct name makes the benchmark's reproducibility intent explicit.

The other RNG changes all look right and are unchanged:

  • pandas.md and polars.md (Series) — genuine legacy np.random.randn to Generator API conversions, and the first rng use in each file.
  • python_by_example.md — the added rng line is necessary here, not redundant. That cell re-quotes the program from (ourfirstprog) and the prose says "the remaining four lines after the import statements", so rng = np.random.default_rng() genuinely is one of those lines. The stale "three lines" count and the line-by-line walkthrough were correctly updated alongside it.
  • numpy.md — the seed=None docstring for DiscreteRV is accurate.
  • autodiff.md — splitting key once into three named subkeys is a real fix, not just cosmetic. The original drew x from key and then split that same key for the shocks, reusing key material.

Also confirmed there are no legacy np.random.* calls left in any of the nine files. The only surviving mention is the illustrative from numpy.random import rand, randn inside the historical SciPy-initialization note in scipy.md, which is a non-executed block quoting old SciPy internals and should stay as is.

The working rule I applied, if it is useful as a convention: re-create rng when a cell is an independent, copy-pasteable example (new section, exercise solution, standalone demo), and rely on the earlier binding when consecutive cells are successive refinements of one program, where the reader is meant to see only what changed.

Reusing the rng name for the seeded benchmark generator is fine: the rename
was a readability preference with no effect on output, and nothing downstream
of the benchmark cell reads rng. The seed is unchanged, so the benchmark data
stays reproducible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mmcky

mmcky commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment: the bench_rng rename in polars.md has been reverted in 9af710c, so the benchmark cell keeps rng = np.random.default_rng(42) as originally written.

Reusing the name is fine here. The rename was a readability preference rather than a fix, it had no effect on output, and nothing downstream of the benchmark cell reads rng. The seed is unchanged, so the benchmark data remains reproducible.

The scipy.md change from that commit stands: the redundant rng re-creation before the linregress example is still removed, and scipy.md still drops out of this PR.

@mmcky

mmcky commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@Chihiro2000GitHub I had a closer look and I actually removed a number of the rng calls -- making use of the general rng object from the beginning of the lecture.

@jstac this is good to go -- bar the final call re: docstring.

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