Speed up DataFrame conversion utilities - #1349
Conversation
- h5_to_gdf: collect per-file DataFrames and concat once instead of concatenating inside the loop (which copies all previously read data on every iteration), and build columns directly from the HDF5 arrays instead of a python list transpose. ~3x at 40 files / 2M rows, with the gap growing in the number of files. - convert_to_gdf: build point geometries with geopandas.points_from_xy instead of one shapely Point per row via apply (44x on 100k rows). - pandas_to_geojson: extract coordinates and properties column-wise instead of per-row iterrows (61x on 5k features). Property values now arrive as plain Python scalars, so integer columns no longer break json serialization of the returned dict. Outputs verified identical against the previous implementations on synthetic data.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesData conversion optimizations
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tick the box to add this pull request to the merge queue (same as
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
leafmap/common.py (1)
16125-16131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle the no-coordinate result before constructing geometry.
If all files are skipped—or the matching group contains no coordinate datasets—
out_df[lon]/out_df[lat]raisesKeyError. Raise a descriptiveValueErroror return a schema-bearing empty GeoDataFrame instead.Suggested guard
out_df = pd.concat(dfs, ignore_index=True) if dfs else pd.DataFrame() +if lat not in out_df.columns or lon not in out_df.columns: + raise ValueError(f"No coordinate data found for dataset {dataset!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leafmap/common.py` around lines 16125 - 16131, Update the GeoDataFrame construction flow to handle an empty or coordinate-less out_df before accessing out_df[lon] and out_df[lat]. When no files contribute coordinate data, raise a descriptive ValueError or return a schema-bearing empty GeoDataFrame; preserve the existing geometry creation for valid coordinate results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@leafmap/common.py`:
- Around line 16117-16120: Update the col_data comprehension in the relevant
data-column construction to replace the separate lat/lon equality checks with a
single membership test against (lat, lon), preserving the existing filtering
behavior while resolving the Ruff warnings.
- Around line 16109-16123: Update the HDF5 file handling loop around `h5py.File`
to use a context manager (`with h5py.File(file, "r") as h5:`), keeping the
existing dataset lookup, column extraction, DataFrame creation, and
missing-dataset behavior unchanged while removing manual close calls.
- Around line 16524-16530: Normalize values in the GeoJSON feature-building flow
before json.dump: replace pandas missing values, including pd.NA and NaN in both
coord_rows and prop_cols, with None; convert datetime-like values to
GeoJSON-compatible strings or numbers. Ensure geometry coordinates and feature
properties use only JSON-native values while preserving the existing feature
structure.
---
Outside diff comments:
In `@leafmap/common.py`:
- Around line 16125-16131: Update the GeoDataFrame construction flow to handle
an empty or coordinate-less out_df before accessing out_df[lon] and out_df[lat].
When no files contribute coordinate data, raise a descriptive ValueError or
return a schema-bearing empty GeoDataFrame; preserve the existing geometry
creation for valid coordinate results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2af2f975-5409-4068-925f-0a747a4a505a
📒 Files selected for processing (1)
leafmap/common.py
Addresses review: h5py file handles are now closed deterministically even if reading a dataset raises, and the coordinate column check uses a single membership test.
|
addressed the review: h5py files now use a context manager so handles close on exceptional paths too, and the membership test is merged per the ruff hint. on the geojson value normalization: the new column-wise extraction produces the same values for NA and datetime cells as the previous iterrows implementation did, so behavior is unchanged there. converting pd.NA to None or formatting timestamps would change existing output, happy to do that as a follow-up if you want it. the build-and-push failure is the fork PR lacking write access to ghcr, not related to the change. |
|
/claude-review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
leafmap/common.py (2)
16525-16531: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove duplicate keys from the feature literal.
propertiesandgeometryare each declared twice; the later values silently replace the earlier ones. Keep only the populated entries to avoid masking future mistakes. (github.com)Proposed cleanup
"type": "Feature", - "properties": {}, - "geometry": {"type": geometry_type, "coordinates": []}, "properties": {prop: values[i] for prop, values in prop_cols.items()}, "geometry": {"type": geometry_type, "coordinates": coords},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leafmap/common.py` around lines 16525 - 16531, Update the feature literal in the GeoJSON feature-building block to declare properties and geometry only once, retaining the populated values based on prop_cols, values, geometry_type, and coords. Remove the earlier duplicate entries while preserving the subsequent geojson["features"].append(feature) behavior.
16121-16129: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle empty or coordinate-less HDF5 results before constructing geometry.
When all files are skipped, or the selected dataset lacks
lat/lon,out_df[lon]andout_df[lat]raiseKeyError. Raise a descriptiveValueErroror return a schema-bearing empty GeoDataFrame before Line 16128.Proposed guard
out_df = pd.concat(dfs, ignore_index=True) if dfs else pd.DataFrame() + if lat not in out_df.columns or lon not in out_df.columns: + raise ValueError(f"No coordinate data found for dataset {dataset!r}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@leafmap/common.py` around lines 16121 - 16129, Update the HDF5 result handling before the GeoDataFrame construction to guard against an empty out_df or missing latitude/longitude columns. Raise a descriptive ValueError, or return a schema-bearing empty GeoDataFrame, before calling gpd.points_from_xy; preserve the existing geometry creation for valid results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@leafmap/common.py`:
- Around line 16525-16531: Update the feature literal in the GeoJSON
feature-building block to declare properties and geometry only once, retaining
the populated values based on prop_cols, values, geometry_type, and coords.
Remove the earlier duplicate entries while preserving the subsequent
geojson["features"].append(feature) behavior.
- Around line 16121-16129: Update the HDF5 result handling before the
GeoDataFrame construction to guard against an empty out_df or missing
latitude/longitude columns. Raise a descriptive ValueError, or return a
schema-bearing empty GeoDataFrame, before calling gpd.points_from_xy; preserve
the existing geometry creation for valid results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc6dadcc-a209-4b90-bf91-184b7a41cea3
📒 Files selected for processing (1)
leafmap/common.py
If every file is skipped or the dataset lacks the lat/lon columns, h5_to_gdf now raises a descriptive ValueError instead of a KeyError from the geometry construction.
|
second round addressed: h5_to_gdf now raises a descriptive ValueError when every file is skipped or the dataset has no lat/lon columns, instead of the KeyError that both the old and new code produced there. on the duplicate-keys comment: the current feature literal declares type, properties, and geometry once each, so I believe that one is looking at a stale diff. happy to adjust if I am missing something. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@leafmap/common.py`:
- Around line 16125-16128: Update the docstring for the surrounding function
containing the lat/lon column validation to document the new ValueError raised
when all files are skipped or the latitude/longitude columns are absent,
alongside the existing filename-related ValueError cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 619e669a-1fdf-485a-b34c-8bcb5402275f
📒 Files selected for processing (1)
leafmap/common.py
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
All three inline comments posted successfully. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- h5_to_gdf: validate lat/lon columns per file instead of only on the concatenated result. Files whose dataset lacks the coordinate columns are now skipped with a message rather than silently contributing rows with NaN coordinates when mixed with files that do have them. - convert_to_gdf: drop the now-unused `Point` import, left over from switching point construction to `gpd.points_from_xy`.
2 participants
Speeds up three conversion utilities in
common.py, outputs unchanged:h5_to_gdf: collect per-file DataFrames and concat once instead of concatenating inside the loop, which copies all previously read data on every iteration. Columns are built directly from the HDF5 arrays instead of a python list transpose. About 3x at 40 files / 2M rows, and the gap grows with the number of files.convert_to_gdf: build point geometries withgeopandas.points_from_xyinstead of one shapely Point per row viaapply(44x on 100k rows).pandas_to_geojson: extract coordinates and properties column-wise instead of per-rowiterrows(61x on 5k features). Property values now arrive as plain python scalars, so integer columns no longer breakjson.dumpof the returned dict.Verified outputs identical against the previous implementations on synthetic data.
pytest tests/test_common.pygives the same results as a clean checkout (the 4 tile-url failures on my machine are unrelated and pre-existing).Built with AI assistance and verified locally.
Summary by CodeRabbit
Performance Improvements
Bug Fixes
New Features