feat(spp_demo): add geographic demo data for Philippines, Sri Lanka, and Togo#44
feat(spp_demo): add geographic demo data for Philippines, Sri Lanka, and Togo#44
Conversation
Summary of ChangesHello @jeremi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant feature by adding geographic demo data for the Philippines, Sri Lanka, and Togo. The changes are well-structured, with a new DemoAreaLoader model to handle data loading, and seamless integration into the existing demo data generator. The inclusion of data files, a new wizard, and comprehensive tests is commendable. I've provided a couple of suggestions to enhance performance and improve robustness in specific areas.
| try: | ||
| with open(geojson_path, encoding="utf-8") as f: | ||
| geojson_data = json.load(f) | ||
| except (OSError, json.JSONDecodeError) as e: | ||
| _logger.warning("Could not read GeoJSON file for %s: %s", country_code, e) | ||
| return 0 |
There was a problem hiding this comment.
The current error handling for GeoJSON files might be confusing if the file is a Git LFS pointer, as json.JSONDecodeError doesn't explain the root cause. It would be more user-friendly to explicitly check for an LFS pointer and log a specific warning, guiding the user to run git lfs pull.
try:
with open(geojson_path, encoding="utf-8") as f:
first_line = f.readline()
if first_line.startswith("version https://git-lfs.github.com/spec/v1"):
_logger.warning(
"GeoJSON file for %s is a Git LFS pointer. "
"Ensure files are checked out with 'git lfs pull' to load shapes.",
country_code,
)
return 0
f.seek(0)
geojson_data = json.load(f)
except (OSError, json.JSONDecodeError) as e:
_logger.warning("Could not read or parse GeoJSON file for %s: %s", country_code, e)
return 0| Area = self.env["spp.area"] | ||
| all_areas = Area.search([]) | ||
| if not all_areas: | ||
| return Area.browse() | ||
|
|
||
| # Find the maximum area_level (deepest in hierarchy) | ||
| max_level = max(all_areas.mapped("area_level")) | ||
|
|
||
| # Get areas at the deepest level | ||
| leaf_areas = all_areas.filtered(lambda a: a.area_level == max_level) | ||
|
|
||
| # Fallback: if no areas at max level, use all areas | ||
| return leaf_areas if leaf_areas else all_areas |
There was a problem hiding this comment.
The current implementation of _get_leaf_areas loads all spp.area records into memory to find the maximum level and then filters them. This could be inefficient for large datasets. You can optimize this by using a database query to find the maximum level and then another query to fetch only the leaf areas.
| Area = self.env["spp.area"] | |
| all_areas = Area.search([]) | |
| if not all_areas: | |
| return Area.browse() | |
| # Find the maximum area_level (deepest in hierarchy) | |
| max_level = max(all_areas.mapped("area_level")) | |
| # Get areas at the deepest level | |
| leaf_areas = all_areas.filtered(lambda a: a.area_level == max_level) | |
| # Fallback: if no areas at max level, use all areas | |
| return leaf_areas if leaf_areas else all_areas | |
| Area = self.env["spp.area"] | |
| # Find the maximum area_level more efficiently | |
| max_level_area = Area.search([], order="area_level desc", limit=1) | |
| if not max_level_area: | |
| return Area.browse() | |
| max_level = max_level_area.area_level | |
| # Get all areas at the deepest level | |
| leaf_areas = Area.search([("area_level", "=", max_level)]) | |
| return leaf_areas |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #44 +/- ##
===========================================
- Coverage 71.31% 41.66% -29.66%
===========================================
Files 299 163 -136
Lines 23618 14591 -9027
===========================================
- Hits 16844 6079 -10765
- Misses 6774 8512 +1738
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…and Togo - Add curated area hierarchies (region, province, municipality) per country - Add GeoJSON boundary shapes for choropleth visualization - Add demo area loader model and wizard for on-demand area data loading - Conditionally load GIS shapes when spp_gis is installed - Integrate geographic areas into demo story generation
Replace the Python-level filter over all areas with a direct ORM query
using the domain [('child_ids', '=', False)], which lets the database
find leaf areas in a single SQL query instead of loading all records
into memory.
jeremi
force-pushed
the
feat/geographic-demo-data
branch
from
1ca1ed1 to
eca97d2
Compare
1 participant
Summary
spp_gisis installedDependencies
spp_gisfeatures for shape loadingOrigin
From
openspp-modules-v2branchclaude/global-alliance-policy-basket.Test plan