abyrd
commented
11 months ago 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.
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.