Graham McIntire
6aa91e7656
Each fix is covered by a regression test that fails on `main` and passes on this commit. Round 1 (initial review): * propagation: thread `latitude` into the conditions map so `score_season/4` actually picks up regional multipliers * hrrr_client / fetcher.rs: `nearest_hrrr_hour` rounds DOWN, never at a future cycle that NOAA hasn't published yet * radio: spherical-vector great-circle midpoint replaces the arithmetic mean — anti-meridian paths no longer fold to Greenwich * weather: `reconcile_weather_statuses` scales the longitude band by `1 / cos(lat)` so the bbox stays ~150 km wide at every latitude * radio/maidenhead: clamp 90°/180° below the field-bucket overflow so `from_latlon` never emits invalid characters like 'S' * prop_grid_rs/pipeline: merge HRRR + NEXRAD-derived rain rates and read `best_duct_freq_ghz` into `best_duct_band_ghz` so the Native Duct Boost actually fires * propagation/region (Elixir + Rust): inclusive upper bounds so points exactly at lat_max get the regional multiplier * weather/sounding_params (Elixir + Rust): drop the 10 m gradient floor so HRRR's thin near-surface layers stop hiding sharp ducts * weather/sounding_params: when the profile ends inside a duct, finalize it with the highest sample as the top instead of throwing it away (Rust port already correct) Round 2 (post-fix sweep): * radio + commercial: single canonical haversine in Radio (atan2 form); Commercial delegates instead of carrying a second copy that could disagree at threshold distances * prop_grid_rs/profiles_file: `snap_coords` matches Elixir's step-aware snap (`round(coord/0.125) * 0.125`, then 3-dp round) so Rust-keyed and Elixir-keyed profile maps land on the same cell * weather/grib2/wgrib2: `parse_lon_val_segment` uses `Float.parse` uniformly — wgrib2 dropping the trailing `.0` from a longitude no longer crashes the whole chain step
27 lines
3 KiB
Markdown
27 lines
3 KiB
Markdown
# Identified Bugs and Observations (Post-Fix Review)
|
|
|
|
This document reflects the state of the codebase after the fixes for the initial 15 bugs were applied.
|
|
|
|
All three items below were verified, covered by failing tests, and fixed on 2026-04-25.
|
|
|
|
## 1. Haversine Implementation Inconsistency — FIXED 2026-04-25
|
|
**Files:** `lib/microwaveprop/radio.ex` vs `lib/microwaveprop/commercial.ex`
|
|
**Description:**
|
|
- `Radio.haversine_km/4` uses the `asin` form: `2 * r * :math.asin(:math.sqrt(a))`
|
|
- `Commercial.haversine_km/4` uses the `atan2` form: `2 * r * :math.atan2(:math.sqrt(a), :math.sqrt(1 - a))`
|
|
**Impact:** While functionally similar, `atan2` is the industry standard for floating-point stability at large distances. Having two different implementations for the same physical calculation can lead to tiny discrepancies (sub-millimeter) that might cause a contact or commercial link to intermittently cross a distance threshold (e.g., 75 km) depending on which module performs the check.
|
|
**Fix:** Switched `Radio.haversine_km/4` to the `atan2` form and made `Commercial`'s private wrapper delegate to it. One canonical implementation, no per-module drift. Numerical-stability regression test added in `test/microwaveprop/radio_test.exs`.
|
|
|
|
## 2. Grid Snapping Mathematical Drift — FIXED 2026-04-25
|
|
**Files:** `lib/microwaveprop/propagation/profiles_file.ex` vs `rust/prop_grid_rs/src/profiles_file.rs`
|
|
**Description:**
|
|
- Elixir `snap/2`: `Float.round(Float.round(lat / step) * step, 3)`
|
|
- Rust `snap_coords`: `(lat * 1000.0).round() / 1000.0`
|
|
**Impact:** The Elixir version performs a division and multiplication by the grid step (`0.125`) before rounding. The Rust version rounds the raw coordinate to the third decimal place. For coordinates that land exactly on a $0.0625$ boundary (the halfway point between cells), these two methods may snap to different cells, causing the UI to report "No Data" or show the wrong factor breakdown for a clicked point.
|
|
**Fix:** Rust `snap_coords` now mirrors the Elixir step-aware snap (`round(coord / 0.125) * 0.125`, then 3-decimal round). Cross-stack parity test in `rust/prop_grid_rs/src/profiles_file.rs` enumerates known-disagreeing inputs.
|
|
|
|
## 3. Potential Exception in Wgrib2 Parsing (Theoretical) — FIXED 2026-04-25
|
|
**File:** `lib/microwaveprop/weather/grib2/wgrib2.ex`
|
|
**Description:** `parse_lon_val_segment/1` uses `String.to_float(lon_str)` for the longitude, but `Float.parse` for latitude and value.
|
|
**Impact:** If `wgrib2` were to ever return an integer string for longitude (e.g. `"235"`) instead of a float (`"235.0"`), `String.to_float` would raise an `ArgumentError`. While `wgrib2` typically outputs floats, using `Float.parse` would be more consistent with the other fields in the same line.
|
|
**Fix:** All three numeric fields now go through `Float.parse/1` (in a `with` chain so a malformed segment returns `nil` instead of bringing down the chain step). Function exposed as `@doc false` and covered by direct unit tests for both decimal-formatted and integer-formatted longitude strings.
|