Repository navigation
fix(geom): non-finite coordinates are no distance, and measures format as an em dash - #956
subarna-kandel wants to merge 1 commit into
Conversation
…t as an em dash A single NaN or infinite vertex used to measure as 20,015,114.442 m -- a quarter of the Earth's circumference -- because haversine_m's h.sqrt().min(1.0) fell into f64::min's NaN rule and turned the broken haversine h into asin(1). The measure tool, the attribute table's Calculate Geometry and the network tools sit under it, and WKB-based files can carry such vertices, so a corrupt file measured silently plausible lengths. Now a non-finite endpoint keeps the haversine result NaN: sums and panels stay visibly broken and comparisons (NaN <= r) exclude the point, while finite edges next to a broken one keep measuring. The formatters format_distance, format_area, format_lonlat and format_dms show an em dash for non-finite values, never "NaN km" or "NaN deg", as the router's formatter already avoided. Fixes opengeos#937. Fixes opengeos#938.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Non-finite measurement propagation crates/geom/src/lib.rs |
haversine_m returns NaN for non-finite coordinates. Tests cover polyline lengths, ring areas, destination longitude, and a finite edge. |
Non-finite value formatting crates/geom/src/lib.rs |
Distance, area, longitude/latitude, and DMS formatters return an en dash for non-finite inputs. Tests also check representative finite formatting. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- opengeos/RustGIS#938 — The issue reports non-finite formatter output; this change displays an en dash for those values.
Possibly related PRs
- opengeos/RustGIS#946 — This PR addresses invalid-coordinate haversine measurements; this change also tests invalid measurement propagation and adds non-finite formatting.
- opengeos/RustGIS#905 — This PR changes
Aeqd::inversehandling for non-finite inputs; this change tests a non-finite destination result and updates haversine handling.
Suggested reviewers: giswqs
Merge Risk: 
5b320
-
Non-finite route inputs are rejected before network snapping, and the intended NaN propagation and dash formatting changes introduce no established merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title is specific and matches the changeset. It names the geom crate and says that non-finite coordinates no longer produce a distance, and that measures format as an em dash. The raw summary conf… Description check The description follows the template. It has Summary, Testing, and Related issue sections. The Summary explains the root cause, the haversine_m fix, the formatter change, and the design choices. The T… Linked Issues check Only #937is a linked issue, and it is closed. Its text is historical context, so the PR is assessed against the current PR description and the PR objective. The reviewed head (5b320fb) confirms the c…Out of Scope Changes check The changes fall into three groups. (1) haversine_mNaN propagation addresses the core of#937directly. (2)format_distance,format_area,format_lonlatandformat_dmsnow return '–' for non…Full details: Title check
-
Explanation
The title is specific and matches the changeset. It names the geom crate and says that non-finite coordinates no longer produce a distance, and that measures format as an em dash. The raw summary confirms both behaviors: haversine_m returns NaN for non-finite inputs, and the four formatters return an en dash for non-finite inputs. The title says "em dash" but the code uses an en dash (–). The title is long at 84 characters, but it is not vague.
Full details: Description check
-
Explanation
The description follows the template. It has Summary, Testing, and Related issue sections. The Summary explains the root cause, the haversine_m fix, the formatter change, and the design choices. The Testing section lists the new tests, the commands run, and the wasm build. The Related issue section is missing. The description says the PR fixes
#937and#938in the Summary, but the template's dedicated Related issue heading is not filled in with a Closes/Part of line.Resolution
Add a "Related issue" section that uses the template form, for example "Closes
#937, closes#938", so the linked issues appear in the expected place.
Full details: Linked Issues check
-
Explanation
Only
#937is a linked issue, and it is closed. Its text is historical context, so the PR is assessed against the current PR description and the PR objective. The reviewed head (5b320fb) confirms the core fix in crates/geom/src/lib.rs.haversine_mnow returnsf64::NANwhen either endpoint is non-finite, before theh.sqrt().min(1.0).asin()guard runs. The issue's expected behaviour allows a refused result or an excluded edge, and never a silently plausible distance. NaN is a visibly refused result.geodesic_length_msums per-edgehaversine_mvalues, so a broken vertex makes the polyline total NaN and does not yield 20,015.114 km.geodesic_ring_area_m2yields NaN for a ring with a non-finite vertex, because its sum propagates NaN. The guard is unchanged for finite input, so antimeridian behaviour from the issue table is preserved. The formatters at head return '–' for non-finite input, so the 'NaN km' strings from the issue no longer appear. Automated tests are described in the PR (a_non_finite_coordinate_is_no_distance_no_area_and_no_place,non_finite_measures_show_a_dash_not_nan). I could not read the test module in the truncated file read, so I rely on the PR description and the +58 line diff summary for those tests. Note: the PR text and the code use the character U+2013 (en dash), though the PR calls it an em dash. This is a naming discrepancy in the description only and does not affect the requirement.
Full details: Out of Scope Changes check
-
Explanation
The changes fall into three groups. (1)
haversine_mNaN propagation addresses the core of#937directly. (2)format_distance,format_area,format_lonlatandformat_dmsnow return '–' for non-finite input. The issue's table notes that the area result reaches the formatters as 'NaN km²' and refers to a companion issue about non-finite formatting. The PR description cites#938for this work.#938is not in the linked issue list, so its tie to this PR depends on the PR description and the#937cross-reference, not on a linked requirement. The formatter change is still a direct response to the symptom in#937, so it is not unrelated. (3) TheViewPort-independent helper changes ingeodesic_interpolateandAeqd::inverseare not part of the diff summary I could verify. TheAeqd::inversenon-finite guard is present at head and returns NaN for non-finite metres, which matches the PR's stated behaviour forgeodesic_destination. I did not assess the behaviour ofgeodesic_interpolatefor non-finite input, because the PR does not claim to change it and the issue does not name it. No unrelated changes were found in the reviewed portions.
-
-
Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks each measured line
Non-finite points return NaN
An en dash marks what cannot be shown
Finite values keep their place
The burrow hops through tested space
And leaves a tidy trail behind
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // pretends otherwise measured a corrupt file's vertex as a quarter of the globe (#937) or, as | ||
| // `0.0`, slid it inside every radius comparison. NaN keeps sums and panels visibly broken. | ||
| if !a.is_finite() || !b.is_finite() { | ||
| return f64::NAN; |
There was a problem hiding this comment.
Bug (medium-high confidence, not run): the NaN result reaches crates/analysis/src/network.rs, which this PR doesn't touch, and there it breaks the snap:
nearest(network.rs ~L99–101) usesmin_by(partial_cmp(...).unwrap_or(Equal)). A NaN distance comparesEqualto everything, andIterator::min_bykeeps the earlier element onEqual. So if a corrupt segment comes first, it stays the "nearest" for the whole search.- The guard
if d > snap_m(L106) is false for NaN, so the route end attaches to that corrupt segment instead of erroring. Before this PR the NaN vertex measured π·R and was rejected.
Suggested fix in network.rs, which is outside this diff so I'm not posting it inline: drop non-finite distances inside nearest (for example let d = haversine_m(q, p); d.is_finite().then_some((i, t, q, d))) and write the snap check as if !(d <= snap_m). Please add a regression test with a NaN vertex in the first segment.
| if !m.is_finite() { | ||
| // A distance that isn't finite isn't a distance (a corrupt geometry can make one): an em | ||
| // dash, never "NaN km", which the router's formatter also avoids. | ||
| return "–".into(); |
There was a problem hiding this comment.
Nit: the character here is U+2013 (en dash), but the title, the comments and the PR body all say "em dash". Either change the wording or use U+2014. Also, tiles/src/routing.rs::format_distance returns "0 m" for NaN, not a dash, so the "behaves like it" claim in the PR body doesn't hold. Pick one convention for the no-value display across both formatters.
Code reviewBugs
Security
Performance
Quality
AGENTS.md
I checked the geometry callers: the Change sizeCompared: the whole PR against main (merge base 342b2b2 to PR head 5b320fb).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
📖 Docs preview
GitHub Pages takes a minute or two to publish a new commit. |
|
@subarna-kandel Can you resolve the merge conflicts? Thanks. |
|
Closing as superseded: |
Summary
Fixes #937 and #938. A single NaN or infinite vertex in a geometry measured as 20,015,114.442 m — a quarter of the Earth's circumference — and non-finite results formatted as
"NaN km","inf km"or"NaN°NaN′0NaN″"in the measure panel, the scale bar and the route errors.haversine_m(crates/geom/src/lib.rs): the haversine guardh.sqrt().min(1.0)turned a NaNhinto1.0(f64::minignores NaN), soasin(1.0) · 2Rmeasured any edge with a non-finite endpoint asπ·R. A non-finite endpoint now keeps the result NaN. Sums, panels and comparisons stay visibly broken (NaN <= ris false, so a corrupt point never slides inside a radius test, unlike a0.0result would), while finite edges next to a broken one keep measuring through their own endpoints.geodesic_length_m, the measure tool (mapview.rs), the attribute table's Calculate Geometry and the network tools sit under it.wgs84_distance_malready propagates NaN through GeographicLib; nothing to change there.format_distance,format_area,format_lonlatandformat_dmsshow an em dash (–) for non-finite values, never"NaN km"or"NaN°".tiles/src/routing.rs::format_distancealready handled non-finite input; the four geometry formatters now behave like it. These are the strings the measure panel (measure_units.rs, metric path), the scale bar (render/src/text.rs), the route/match errors (analysis/src/network.rs) and the status bar display.Choices in this PR
.min(1.0). The guard's job is to catch floating-pointhvalues that round marginally above 1.0, and it does that fine for finite input; the wrongness came from non-finite input reaching the formula at all.map/src/projection.rsa cap-domain test ishaversine_m(center, c) <= radius…: a0.0result would put a non-finite point inside every cap. NaN excludes it, and the measure panel shows–(with the formatter half of this PR) rather than a plausible zero.222,390.160 mfor 179 → -179 and 179 → 181), so nothing about unwrapped longitudes changes here.geodesic_ring_area_m2already returns NaN for a ring with a broken vertex; with the formatter change that now displays as–instead of"NaN km²".Testing
rustgis-geom:a_non_finite_coordinate_is_no_distance_no_area_and_no_place: NaN and ±inf endpoints give NaN throughhaversine_mandgeodesic_length_m; a ring with a broken vertex gives NaN area;geodesic_destinationwith a broken distance gives NaN; the finite edge next to a broken one still measures111,195.080 m(1° at the equator on the mean sphere);non_finite_measures_show_a_dash_not_nan: the four formatters show–for NaN, +inf and -inf, and the finite cases format exactly as before (850 m,12.5 km,2.5 km²,12.3456° W 45.6789° N).haversine_m((0,0) → (NaN,0)) == 20,015,114.442— now measures NaN.cargo test -p rustgis-geom,cargo fmtandcargo clippy -p rustgis-geom --all-targetspass; the wasm target builds (cargo build --target wasm32-unknown-unknown -p rustgis-geom).Summary by CodeRabbit