WGS84 to Mercator conversion stability #892
Conversation
Handle edge cases where the WGS84 envelope lies on Mercator pixel edges. Handle numerical instability in repeated coordinate system conversions. Add tests for repeated coordinate conversion.
Trimming when handling possibly recycled WGS84 bounds. Buffering when supplying tight fit opportunity bounding boxes.
WebMercatorGridPointSet methods now call static Grid functions with zoom. All functions are now using standard Java Math (not FastMath).
abyrd
force-pushed
the
wgs-mercator-stability
branch
from
9c4c98b
to
1d70428
Compare
Also check stablity of single-pixel extents.
| // Find width and height in whole pixels, handling the case where the right envelope edge is on a pixel edge. | ||
| int height = (int) Math.ceil(latToFractionalPixel(wgsEnvelope.getMinY(), zoom) - north); | ||
| int width = (int) Math.ceil(lonToFractionalPixel(wgsEnvelope.getMaxX(), zoom) - west); |
There was a problem hiding this comment.
Note that this is a key part of the PR, using ceil() to handle the case where the projected value already lies exactly on a pixel boundary.
|
Coming back to this today: It should be possible to split this into two PRs: One PR to add the repeated-conversion test and the minimal set of changes to make it pass; A second PR to merge the WGS84-Mercator conversion functions present on different classes. The first PR might be easier to review and safer to get into a release without the changes in the second one. And the second one might be better combined with the complete elimination of the wrapper conversion functions as in #894, possibly even creating a third PR to finally switch to the pixel-center functions for destinations. |
|
Closing this PR since I've split it into three separate changes. The minimal replacement for this PR is #897. |
As discussed in today's call: Regional request bounds are specified in WGS84 coordinates. Repeating a regional analysis with the same bounds as an existing one involves the UI looking at the web Mercator bounds of an existing analysis and converting them to WGS84 to create the new regional analysis request. This creates a feedback loop between WGS84 and web Mercator where WGS84 bounding boxes lie exactly on the edges of Mercator pixels, creating a literal edge case for the function com.conveyal.r5.analyst.WebMercatorExtents#forWgsEnvelope and populateTask.
I have revised the way web Mercator extents are created to handle the edge case. This is further reinforced with additional factory methods that should handle any numerical instability or lack of precision in the repeated conversions.
The eventual real fix for this is #893.
I also combined all the duplicate methods for WGS84 and Mercator conversions so we don't have two slightly paraphrased copies of each one. The functions were mathematically identical but expressed a little differently which involved some merging, so when reviewing this please help me keep an eye out for any algebraic errors in the math functions or errors in reasoning about use of truncation, ceiling function etc.
I verified repeatedly that the merged functions had the same signatures (parameter and return types), equivalent order of operations etc. but it's always good to have additional pairs of eyes on such things.
Combining these equivalent functions might seem a little off topic for this PR. However, both here and in subsequent work I need to exhaustively check whether we are getting pixel edges or centers. It's harder to be sure you've found all the references in the codebase when the functions to do so are duplicated or wrapped.