Sanitize Copernicus DEM samples, fix tile overlap logic, preserve 2-node highways, and allow env example files#6
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
There was a problem hiding this comment.
All changes in this PR are well-implemented and thoroughly tested. The elevation sanitization logic correctly handles nodata values, the tile overlap algorithm properly clamps to global bounds, and the minimum node count logic appropriately preserves 2-node highways while filtering degenerate geometries. The comprehensive unit tests validate all the critical functionality.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
Code Review
This pull request updates the .gitignore to include environment example files and enhances the Copernicus DEM provider by implementing elevation normalization and a more precise tile overlap calculation. Additionally, it adjusts Overture data processing to allow two-node road segments while maintaining a three-node minimum for other features. Review feedback suggests optimizing the tile range logic by using inclusive ranges to improve efficiency and handle edge cases where the bounding box is outside valid bounds.
| fn overlapping_tiles(bbox: &LLBBox) -> Vec<(i32, i32)> { | ||
| let lats = overlapping_tile_range(bbox.min().lat(), bbox.max().lat(), -90, 89); | ||
| let lngs = overlapping_tile_range(bbox.min().lng(), bbox.max().lng(), -180, 179); | ||
|
|
||
| lats.into_iter() | ||
| .flat_map(|lat| lngs.iter().copied().map(move |lng| (lat, lng))) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Since overlapping_tile_range now returns a range, overlapping_tiles can be simplified to avoid intermediate Vec allocations for latitudes and longitudes. Note that we still collect() the final result because the caller in fetch_raw (line 104) requires the length of the tile list for progress reporting.
fn overlapping_tiles(bbox: &LLBBox) -> Vec<(i32, i32)> {
let lats = overlapping_tile_range(bbox.min().lat(), bbox.max().lat(), -90, 89);
let lngs = overlapping_tile_range(bbox.min().lng(), bbox.max().lng(), -180, 179);
lats.flat_map(|lat| lngs.clone().map(move |lng| (lat, lng)))
.collect()
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Motivation
NaNto avoid corrupting downstream grids.!.env.exampleand!.env.*.examplefrom.gitignore.Description
normalize_elevation_sampleto filter out non-finite and out-of-range elevation samples and replaced raw conversions with calls to this function when decoding TIFF samples.overlapping_tile_rangeandoverlapping_tilesto compute overlapping 1°×1° tile coordinates using proper floor/ceil logic and clamping to[-90,89]/[-180,179]limits, and replaced the previous naive loops withoverlapping_tiles.MIN_VALID_ELEVATION_MandMAX_VALID_ELEVATION_Msanity bounds and updated TIFF decoding paths to handle bothF32andF64decoding results while normalizing values.min_clipped_node_countto return2for ways tagged as highways and3otherwise, and used it to decide whether a clippedProcessedWayshould be kept..gitignoreto allow committingenvexample files via!.env.exampleand!.env.*.exampleentries.normalize_elevation_sample,overlapping_tiles, andmin_clipped_node_countand kept existing provider and parser tests.Testing
cargo testwhich executed provider and parser unit tests including the newly added tests and they all passed.test_normalize_elevation_sample_keeps_valid_values,test_normalize_elevation_sample_filters_invalid_values,test_overlapping_tiles_excludes_integer_max_edge,test_overlapping_tiles_clamps_global_edges, andtest_min_clipped_node_count_keeps_two_node_roads, and they succeeded.Codex Task