Skip to content

fix(create-chapter): Shanghai map-dot override + reposition 22 stale chapter dots - #11

Merged
rparundekar merged 3 commits into
mainfrom
fix/shanghai-map-dot-override
Jul 7, 2026
Merged

fix(create-chapter): Shanghai map-dot override + reposition 22 stale chapter dots#11
rparundekar merged 3 commits into
mainfrom
fix/shanghai-map-dot-override

Conversation

@rparundekar

@rparundekar rparundekar commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Summary

Slide 5 ("THE NETWORK") of each chapter's Event Template/Slides.pptx carries a green "you-are-here" dot + label that ships parked at San Francisco. 22 chapters created before the auto-place feature (#8) still had that dot at the SF default even though their city text was correct. This PR adds the one projection fix that was missing (Shanghai) so the cloner places it correctly going forward; the 22 stale chapters were repaired as a one-time data fix directly in Drive.

Changesets

1. fix(create-chapter): add Shanghai pixel override for slide-5 map dot

Files: skills/aaif-create-chapter/scripts/create_chapter.py
Shanghai (~121°E) is in the map's distorted western-Pacific zone. The linear formula lands it ~8px offshore; pinned to the Chinese coast at pixel (833, 330), verified by rendering the projected dot on image18.png.

2. fix: address self-review findings

Files: create_chapter.py, test_create_chapter.py

  • Corrected/clarified the Shanghai comment (small coastline nudge, distinct from the gross Oceania distortion).
  • main() now prints a Note when the city uses a fixed pixel override, so the reported Coords: line isn't mistaken for where the dot lands.
  • Added test_every_override_pixel_is_within_the_map — a table-wide bounds invariant guarding all override pixels.

3. fix: address PR review feedback

Files: create_chapter.py, test_create_chapter.py
Copilot: fixed-pixel-override cities were gated on geocoding succeeding, so one would fall back to San Francisco if Nominatim was unreachable — despite being placed by name. Added map_dot_latlon() (sentinel for override cities without explicit --lat/--lon), so they place without a network lookup and skip the SF warning. Covered by 3 new tests.

Context (data repair, not in this diff)

The projection was validated by re-deriving the dots of already-correct chapters (London, Berlin, New York, Amsterdam, Chicago, Seoul) — reproduced to within ~65 EMU (sub-pixel). The 22 stale chapters (incl. Washington DC and Tel Aviv, which needed no code change — the existing linear projection places them correctly) were repositioned and re-uploaded to Drive, then re-read to confirm.

Test Plan

  • pytest test_create_chapter.py — 27 passed
  • All 22 repositioned dots verified off the SF default and on-target in Drive
  • Override city stays placeable when geocoding is unavailable (no SF fallback)
  • Spot-check a couple of chapter decks' slide 5 in Google Slides

Generated using hero-skills.

Shanghai (~121°E) sits in the map's distorted western-Pacific zone where
the separable lon/lat projection can't place cities (same reason Seoul,
Sydney, and Melbourne already have overrides). The linear formula lands
it ~10px out to sea; pin it to the Chinese coast at pixel (833, 330).

Verified against image18.png by rendering the projected dot on the map.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rparundekar

Copy link
Copy Markdown
Collaborator Author

Self-Review

Ran all five pr-review-toolkit agents. No critical or important issues — the change is a minimal, in-convention data addition and the coordinate is internally consistent with both projection formulas (lon2x(121.47)≈841 → override x=833 nudges ~8px west; override y=330 ≈ lat2y(31.23), so only x moves).

Suggestions (4)

  • [comment-analyzer / code-reviewer] create_chapter.py:218 — comment says Shanghai lands "~10px out to sea," but the actual formula displacement is ~8px. Tighten the figure.
  • [comment-analyzer] create_chapter.py:214-220 — the Shanghai note is spliced into the paragraph about the large Oceania distortion (Sydney/Melbourne dragged ~50-68px). Shanghai is a small ~8px cosmetic coastline nudge and is genuinely west of Tokyo — set it apart so the two cases aren't conflated.
  • [pr-test-analyzer] test_create_chapter.py — a Shanghai-specific equality assert would just mirror the constant (no value). A table-wide bounds invariant (every PIXEL_OVERRIDES pixel within 0..MAP_PX) would catch off-map/transposed coords for all cities — worth adding.
  • [silent-failure-hunter] create_chapter.py:485-488 (pre-existing) — for an override city, the printed Coords: line reports geocoded/--lat/--lon values that are then discarded by project_city, so the number shown isn't where the dot lands. Affects Seoul/Sydney/Melbourne today; this PR extends it to Shanghai. Consider a note when the city is an override city.

Strengths

  • Comment explains why (non-recoverable from code, since project_city short-circuits the formula for override cities).
  • Follows the exact override-table format and name-match convention; type/shape identical to existing entries.
  • Mechanism already covered by existing parametrized override tests (23 passing).

Generated using hero-skills.

- Correct the Shanghai comment (~10px -> ~8px offshore) and set it apart
  from the gross Oceania distortion it was spliced into
- Print a Note when a city uses a fixed pixel override, so the reported
  Coords line isn't mistaken for where the dot lands
- Add a table-wide test that every PIXEL_OVERRIDES pixel is within the map

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rparundekar

Copy link
Copy Markdown
Collaborator Author

Self-Review — Improvements

Suggestions (4 / 4 addressed):

  • create_chapter.py:218 — "~10px out to sea" → "~8px offshore" — corrected the figure to match lon2x(121.47)≈841 vs override 833.
  • create_chapter.py:214-220 — reworded so Shanghai's small cosmetic coastline nudge is set apart from the gross Sydney/Melbourne Oceania distortion (no longer conflatable).
  • create_chapter.py:485-491main() now prints a Note when the city is in PIXEL_OVERRIDES, so the reported Coords: line isn't mistaken for the dot location; fixes the misleading output for Seoul/Sydney/Melbourne/Shanghai.
  • test_create_chapter.py:105-113 — added test_every_override_pixel_is_within_the_map, a table-wide bounds invariant catching off-map/transposed override pixels for all cities. Suite now 24 passing.

Skipped:

  • Case-insensitive / trimmed override key matching — broader design change; the whole codebase matches on canonical city names, so left as-is.

Commits: d03691c


Generated using hero-skills.

@rparundekar
rparundekar marked this pull request as ready for review July 7, 2026 18:33
Copilot AI review requested due to automatic review settings July 7, 2026 18:33

Copilot AI left a comment

Copy link
Copy Markdown

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 updates the aaif-create-chapter cloner’s slide-5 (“THE NETWORK”) map-dot placement logic by adding a Shanghai fixed-pixel override, improving CLI output clarity when a fixed override is used, and adding a table-wide invariant test to prevent out-of-bounds overrides.

Changes:

  • Added a Shanghai (x, y) entry to PIXEL_OVERRIDES for slide-5 map-dot placement.
  • Updated main() output to print an explicit note when a city’s dot uses a fixed pixel override (so Coords: isn’t misread as the placement driver).
  • Added a unit test that asserts all override pixels fall within the map image bounds.

Reviewed changes

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

File Description
skills/aaif-create-chapter/scripts/create_chapter.py Adds Shanghai override and prints a “fixed pixel override” note in CLI output.
skills/aaif-create-chapter/scripts/test_create_chapter.py Adds a bounds invariant test covering all entries in PIXEL_OVERRIDES.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread skills/aaif-create-chapter/scripts/create_chapter.py Outdated
Copilot: fixed-pixel-override cities were gated on geocoding succeeding,
so an override city (e.g. Shanghai) would fall back to San Francisco if
Nominatim was unreachable — even though its dot is placed by name and
never needs coordinates.

- Add map_dot_latlon(): returns a sentinel for override cities when no
  explicit --lat/--lon is given, so they're placeable without a network
  lookup and skip the "stays at San Francisco" warning
- main() uses it and prints a clear "fixed pixel override" coords line
- Cover the new helper (placeable-without-geocode, non-override still
  None, explicit coords win)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@rparundekar

Copy link
Copy Markdown
Collaborator Author

Respond-to-Comments Improvements

Code changes (1 applied):

  • create_chapter.py:489 (Copilot) — fixed-pixel-override cities were gated on geocoding succeeding, so an override city would fall back to San Francisco if Nominatim was unreachable — even though its dot is placed by name. Added map_dot_latlon() returning a sentinel for override cities without explicit --lat/--lon, so they're placeable with no network lookup; main() prints a clear "fixed pixel override" coords line instead of the SF warning. Covered by 3 new tests (27 passing).

Commits: 5ee3fe6

Remaining unresolved threads: none


Generated using hero-skills.

@rparundekar
rparundekar merged commit f115f4e into main Jul 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants