fix(chart): updates counties of kenya map#38019
Conversation
|
Bito Review Skipped - No Changes Detected |
|
Thanks for picking this up — the old map really was broken (per #34100), and aligning with ISO 3166-2:KE ( Two things I'd like to nail down before approving:
Once those are addressed I'm happy to approve. |
af9dc2e to
04c606e
Compare
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@rusackas branch is updated! |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM, but technically a breaking change for anyone who has maps with the current countries. Will add this ton the proposals for the 7.0 breaking change window, happening imminently.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #38019 +/- ##
=======================================
Coverage 65.15% 65.15%
=======================================
Files 2789 2789
Lines 157543 157543
Branches 35864 35864
=======================================
Hits 102644 102644
Misses 52931 52931
Partials 1968 1968
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Breaking window is now open, so we can merge this! But... the notebook is conflicting again. If you see a clean way to update/rebase it so both your changes and the upstream changes survive here, I'm happy to merge. I can try to do so soon-ish, but probably won't be immediate... I'd appreciate the help. |
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
villebro
moved this from Propose for Lazy Consensus
to Lazy Consensus Reached
in Apache Superset 7.0
04c606e to
c544e1d
Compare
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| "execution_count": null, | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": "kenya_copy = df[df.admin == 'Kenya'].copy()\n# Source: geoBoundaries Kenya ADM1 (CC BY 4.0) via OCHA HDX\n# https://data.humdata.org/dataset/geoboundaries-admin-boundaries-for-kenya\nkenya_geojson_url = \"https://github.com/wmgeolab/geoBoundaries/raw/9469f09/releaseData/gbOpen/KEN/ADM1/geoBoundaries-KEN-ADM1.geojson\"\n\ntry:\n kenya_gdf = gpd.read_file(kenya_geojson_url)\n kenya_gdf = kenya_gdf.rename(columns={'shapeName': 'name', 'shapeISO': 'iso_3166_2'})\n kenya_copy = gpd.GeoDataFrame(\n kenya_gdf,\n geometry='geometry',\n crs=kenya_gdf.crs\n )\n \n print(\"GeoJSON file for Kenya downloaded and loaded successfully.\")\nexcept Exception as e:\n print(f\"Unable to download or load the GeoJSON file for Kenya. Error: {str(e)}\")\n print(\"Please download the file from the URL and try again.\")\n\nnot speed_run and kenya_copy.plot(**plot_styles)" |
There was a problem hiding this comment.
Suggestion: The error handler catches all exceptions and then continues execution, which means Kenya will silently fall back to the old Natural Earth geometry if the download/load fails, defeating this fix while still producing output. Fail fast (or explicitly stop adding Kenya to the mapping) when the new GeoJSON cannot be loaded. [logic error]
Severity Level: Major ⚠️
- ❌ Kenya legacy country map may retain outdated boundaries.
- ⚠️ Generated Kenya GeoJSON inconsistent with 2010 counties.
- ⚠️ Notebook run appears successful despite Kenya data failure.Steps of Reproduction ✅
1. Open the Jupyter notebook at
`superset-frontend/plugins/legacy-plugin-chart-country-map/scripts/Country Map GeoJSON
Generator.ipynb` and run cells up to the Kenya section defined at line 2397 (cell with
`kenya_copy = df[df.admin == 'Kenya'].copy()` and the `try`/`except` around
`gpd.read_file(kenya_geojson_url)`).
2. Execute this Kenya cell in an environment where the GitHub URL
`geoBoundaries-KEN-ADM1.geojson` is unreachable (e.g., no internet or blocked GitHub),
causing `gpd.read_file(kenya_geojson_url)` in the `try` block at line 2397 to raise an
exception.
3. Observe that the `except Exception as e:` block at line 2397 catches the error, prints
the failure messages, and then completes the cell without modifying `kenya_copy`, which
still holds the original Natural Earth geometry from `df[df.admin == 'Kenya'].copy()`.
4. Run the later cell that builds the country mapping dictionary at lines 3954–3957, where
`"kenya": kenya_copy,` is added; subsequently running the export cells (below this hunk in
the notebook) will generate GeoJSON for Kenya using the stale Natural Earth boundaries
while the notebook execution appears successful.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-country-map/scripts/Country Map GeoJSON Generator.ipynb
**Line:** 2397:2397
**Comment:**
*Logic Error: The error handler catches all exceptions and then continues execution, which means Kenya will silently fall back to the old Natural Earth geometry if the download/load fails, defeating this fix while still producing output. Fail fast (or explicitly stop adding Kenya to the mapping) when the new GeoJSON cannot be loaded.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch, fixed. The except now re-raises instead of silently falling back to the old Natural Earth geometry.
|
The flagged issue is correct. The current implementation catches exceptions during the GeoJSON download and load process but continues execution, resulting in the use of stale Natural Earth geometry. To resolve this, you should raise an exception or explicitly clear the superset-frontend/plugins/legacy-plugin-chart-country-map/scripts/Country Map GeoJSON Generator.ipynb |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Looking good to me, but double checking if we can safely use this data under the source's licensing. |
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1636d6f to
5fe0e8c
Compare
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ceecb75 to
916016a
Compare
- Update Kenya GeoJSON source in notebook to geoBoundaries (CC BY 4.0), pinned to commit 9469f09 for reproducibility, replacing the previous personal GitHub repo URL with an authoritative, licensed source - Add column rename (shapeName→name, shapeISO→iso_3166_2) to match the save_geojson() function expected schema - Add UPDATING.md entry documenting the key-space change from old province codes (KE-110…KE-800) to ISO 3166-2:KE county codes (KE-01…KE-47) Addresses reviewer feedback on PR apache#38019. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Re-raise the download/load error instead of silently swallowing it, so the notebook doesn't produce stale Natural Earth boundaries for Kenya while looking like it succeeded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The notebook and code comment claimed the geoBoundaries Kenya ADM1 boundaries are CC BY 4.0. geoBoundaries' own per-boundary metadata (https://www.geoboundaries.org/api/current/gbOpen/KEN/ADM1/) lists this specific dataset as Public Domain, sourced from RCMRD / Africa GeoPortal, not geoBoundaries' project-wide CC BY 4.0 terms. Also corrects a county name geoBoundaries truncates ("Tharaka" -> "Tharaka-Nithi"), and adds an explicit simplify factor for Kenya so the generated file lands in the same size range as comparable countries (previously ballooned to ~8MB unsimplified since Kenya's bounding-box size narrowly misses the notebook's existing simplification threshold). kenya.geojson is regenerated accordingly (160KB, 47 valid counties). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
916016a to
05e2e91
Compare
|
Thanks for this PR! It'll be in Superset 7.0 :D |
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
SUMMARY
This PR updates the administrative areas of the Kenya country map visualization to reflect redrawn counties as of 2010. As discussed here, Natural Earth has not been updated in quite some time and it seems best to adjust the map in this way.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
Access a dataset with modern ISO 3166-2:KE codes and attempt to plot data on the Kenya country map.
ADDITIONAL INFORMATION