Skip to content

[hoist_failure] Clean up the code and correct minor errors - #656

Merged
jstac merged 4 commits into
mainfrom
clean_hoist
Aug 5, 2026
Merged

[hoist_failure] Clean up the code and correct minor errors#656
jstac merged 4 commits into
mainfrom
clean_hoist

Conversation

@HumphreyYang

Copy link
Copy Markdown
Member

This PR cleans up the code and correct minor errors and typos across the lecture.

It also improves the printing and smooth some of the sentences.

@github-actions

Copy link
Copy Markdown

📖 Netlify Preview Ready!

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

📚 Changed Lecture Pages: hoist_failure

@github-actions

Copy link
Copy Markdown

📖 Netlify Preview Ready!

Preview URL: https://pr-656--sunny-cactus-210e3e.netlify.app (174da1f)

📚 Changed Lecture Pages: hoist_failure

@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-656--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-656--sunny-cactus-210e3e.netlify.app/hoist_failure.html

This PR touches: hoist_failure.md. Last CI build: success@2025-11-14. Branch: 166 commits behind main as of 2026-07-08.

@Chihiro2000GitHub

Copy link
Copy Markdown
Contributor

Hi @HumphreyYang, I'm working through the NumPy random API migration in QuantEcon/meta#299, and hoist_failure.md is one of the lectures still on the list.

This PR already rewrites the sampling cell, so rather than open a separate PR that changes the same lines, I thought I would ask here first. While you are in there, would you be happy to use the Generator API for those draws?

rng = np.random.default_rng(1234)
s1 = rng.lognormal(μ, σ, n_samples)
s2 = rng.lognormal(μ, σ, n_samples)
s3 = rng.lognormal(μ, σ, n_samples)

That keeps the seed you added, and it saves us both from a conflict on the same lines.

If you would rather keep this PR as it is, I am happy to do the migration in a separate PR once this one is merged.

Brings the branch up to date after 218 commits. Main has not touched
hoist_failure.md since this branch opened, so the merge is clean.

On top of that:

- use np.random.default_rng for the lognormal draws instead of
  np.random.seed plus the legacy np.random.lognormal, as requested by
  @Chihiro2000GitHub for the meta#299 migration and required by the
  style guide's NumPy random section. The seed added by this branch is
  preserved.
- add mystnb captions and names to all seven figures, which the figures
  style guide requires
- make the install cell `{code-cell} ipython3` with `:tags:` syntax,
  matching the other 22 cells in the lecture

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

jstac commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Thanks @Chihiro2000GitHub, and sorry for the slow reply — @HumphreyYang hasn't been able to get back to this one, so I've picked it up.

Yes, let's do it here rather than in a separate PR. I've made exactly the change you suggested on this branch:

rng = np.random.default_rng(1234)
s1 = rng.lognormal(μ, σ, n_samples)
s2 = rng.lognormal(μ, σ, n_samples)
s3 = rng.lognormal(μ, σ, n_samples)

It replaces the np.random.seed(1234) plus legacy np.random.lognormal that this branch had introduced, and keeps the seed. That's the only sampling in the lecture, so hoist_failure.md should be clear for meta#299 once this merges — worth ticking off your list rather than opening a follow-up.

Asking first was the right call: this branch rewrites that cell substantially, so a parallel PR would have conflicted on exactly those lines.

While updating the branch I also merged current main into it (it was 218 commits behind, which was why the preview was 404ing), added the mystnb captions the figures style guide asks for, and brought the install cell into line with the other cells. The figures will shift slightly because PCG64 draws a different stream from the legacy RandomState — shape and magnitude are unchanged, which is the expected outcome noted in the migration guidance.

@jstac

jstac commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Opened #1027 for the two new exercises — exercise 2 can't be answered as posed (the comparison it asks for holds by linearity of expectation regardless of the rare event approximation), and exercise 1's conclusion of $p = 13$ disagrees with the $p = 15$ used in the lecture body. Neither is a regression against main, so this PR needn't be blocked on them.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

📖 Netlify Preview Ready!

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

Commit: e717c0a

📚 Changed Lectures


Build Info

@jstac
jstac marked this pull request as ready for review August 5, 2026 11:21
@jstac jstac added review and removed in-work labels Aug 5, 2026
@jstac
jstac merged commit a7aafd5 into main Aug 5, 2026
2 checks passed
@jstac
jstac deleted the clean_hoist branch August 5, 2026 19:06
@Chihiro2000GitHub

Copy link
Copy Markdown
Contributor

Thank you @jstac for picking this up!

I really appreciate you making the change I suggested -- that's very helpful. I've noted this in QuantEcon/meta#299.

Thanks again!

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