Repository navigation
Conversation
…ellipsoid and an unread GeoTIFF transformation #1478's fixtures computed Mars and Earth equidistant cylindrical metres on a sphere and used GeoTIFF CT 23 as an unread transformation. #1463, merged alongside, computes eqc on the ellipsoid (EPSG 1028) and reads CT 23 as Robinson, so both tests failed on main on every platform.
giswqs
added
the
ci:full
|
Caution Review failedThe pull request is closed. Walkthrough
Merge Risk:
 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // The same points projected on the planetographic ellipsoid, as RustGIS computes an | ||
| // equidistant cylindrical projection on an ellipsoid (EPSG method 1028). | ||
| let f = 1.0 / 169.894447223612; | ||
| let on_ellipsoid = rustgis_geom::EquidistantCylindrical::new(r, f * (2.0 - f), 0.0, 0.0, 0.0, 0.0, 0.0).unwrap(); |
There was a problem hiding this comment.
Quality / test coverage (confidence: medium). The new fixture metres come from rustgis_geom::EquidistantCylindrical, the same code the reader uses, so the round trip cannot catch an error in that projection. The PR body says whether reading eqc on an ellipsoid is right is still open (#1586), and these assertions now pin that behaviour. Consider one hard-coded reference value per datum, worked out independently (for example from the GN7-2 formula, or a published PROJ result), next to the computed fixtures. Left for the release quality pass if you prefer; it doesn't block the CI fix.
| assert_eq!(said(&src.info().notes), 1, "{:?}", src.info().notes); | ||
| let a = 6_378_137.0f64; | ||
| let earth_metres = LONLAT.map(|(lon, lat)| (a * lon.to_radians(), a * lat.to_radians())); | ||
| let f = 1.0 / 298.257223563; |
There was a problem hiding this comment.
Nit (confidence: high, left for the release quality pass). This let f shadows the earlier let f in the same function, and the two blocks that turn lon/lat into ellipsoidal metres are near-copies. A small helper, such as fn ellipsoid_metres(a: f64, inv_f: f64, lonlat: [(f64, f64); 2]) -> [(f64, f64); 2], would remove both.
Code reviewNo bugs, security issues, or AGENTS.md violations found. The change is test-only (one file, Bugs: none found. Security: none found. Performance: none found. Quality (left for the release quality pass per AGENTS.md; neither blocks the CI fix):
AGENTS.md: compliant. Test-only change, no new dependency, no user-visible text, and Prior reviews: no existing threads, so none were left alone. Change sizeCompared: the whole PR against main (merge base d866893 to PR head 027439d).
|
Fixes the red CI on main (run 42443783322, every
testjob).Cause
Two PRs merged close together and their tests disagree:
crates/data/tests/planetary_crs.rs. Its fixtures computed equidistant cylindrical metres on a sphere of the semi-major axis, and used GeoTIFF coordinate transformation 23 as one RustGIS doesn't read (the comment called it the New Zealand Map Grid).eqcon an ellipsoid with EPSG method 1028's ellipsoidal formulas, and read CT 23 as Robinson (which it is in the GeoTIFF spec; the New Zealand Map Grid is 26).Failures on all four platforms:
planetographic_systems_without_east_axes_are_refused:eqc_easting: Coord { x: -133.8, y: 18.859… }instead of 18.65a_mars_projection_in_a_geotiff_is_said_or_refused:unwrap_err()on anOkraster (CT 23 now read)Fix
Test-only. The fixtures projected on the Mars 2015 planetographic ellipsoid (three cases) and on WGS 84 (the Earth westing case) now compute their metres with
rustgis_geom::EquidistantCylindrical, as main reads them; the sphere cases are unchanged. The unread-transformation case uses CT 26 (New Zealand Map Grid). What each test checks (axes, refusals, planetary notices) is unchanged.Whether reading
eqcon an ellipsoid differently from PROJ/GDAL (which compute it on a sphere) is right is a behaviour question, filed as #1586.Evidence
cargo test -p rustgis-data --test planetary_crs: 17 passed (2 failed on main). fmt and clippy pass.Summary by CodeRabbit